Just looking for Feedback

Here was the problem I was attempting to solve with this program. Program: Write a program that takes in 50 values and prints out the highest, the lowest, the average and then all 50 input values, one per line.

Below is my solution, just wondering if you all see any errors, or if you would have done anything different... Thank you for your time!

Note: I have only gotten up to the complexity of arrays, I am still learning so if this seems basic its probably because I don't know a more complex/easier method as of yet. This was a problem in the "arrays" chapter of the book.

Code:

---

#include <cstdlib>
#include <ctime>
#include <iostream>


using namespace std;


int array[50];
int highestValue = array[0];
int lowestValue = array[0];
int findHighest (int array[], int size)
{
    for (int i = 0; i < size; i++)
    {
        if ( array[ i ] > highestValue)
        {
            highestValue = array[i];
        }
    }
    return highestValue;
}
int findLowest (int array[], int size)
{
    for (int i = size; i > 1; i--)
    {
        if ( array[ i ] < lowestValue)
        {
            lowestValue = array[i];
        }
    }
    return lowestValue;
}
int findAverage (int array[], int size)
{
    int sum = 0;
    for (int i = 0; i < size; i++)
    {
        sum += array[i];
    }
    return sum/50;
}
void displayArray (int array[], int size)
{
    cout << "{";
    for (int i = 0; i < size; i++)
    {
        if ( i != 0 )
        {
            cout << "\n";
        }
        cout << array [i];
    }
    cout << "}";
}


int main ()
{
    srand(time(NULL));
    for ( int i = 0; i < 50; i++ )
    {
        array[i] = rand() % 100;
    }
    cout << "Highest Value: " << findHighest (array,50) <<"\n";
    cout << "Lowest Value: " << findLowest (array,50) <<"\n";
    cout << "Average Value: " << findAverage (array,50) <<"\n";
    cout << "Array: ";
    displayArray(array, 50);
    cout << "\n";
    return 0;
}

---

You initialize highestValue and lowestValue to a junk value in the first position of the array.

Note: Having highestValue and lowestValue as Global Variables is also poor style/design.

Note: Having array being global might be considered OK; but, I would NOT make it global in most cases.

Code:

---

int array[50];
int highestValue = array[0];
int lowestValue = array[0];

---

Tim S.

I notice that you have three global variables:

Code:

---

int array[50];
int highestValue = array[0];
int lowestValue = array[0];

---

array should be local to the main function, then passed around as needed. highestValue and lowestValue should be local to findHighest and findLowest, respectively.

Also, since you have the using directive using namespace std; you should not use the name array as it may conflict with std::array. An alternative name like numbers could work.

Also, you are doing in different loops, what you can easily do in a single iteration.
That may not look so structured in the form of functions.. but it sure works better.

The function findAverage has the magic number 50 in it; this is poor style and design.

http://en.wikipedia.org/wiki/Magic_n...ical_constants

Tim S.

The standard library already have algorithms for finding max and min in a set of elements already, actually. You may want to try getting familiar with them as an exercise next.
They're called min_element and max_element, found in <algorithm>.
STL Algorithms - C++ Reference

If it were me, the number 50 would only appear in one place in the entire program.
There are various ways to ensure that, just pick one that works for you.

Apart from things already mentioned (not using statics, using standard algorithms, avoiding magic values) I also wouldn't even use an array. I would employ one of the standard containers. I'd also make use of a stream inserter for the output.

Doing all those things would give code that almost certain works better, is much briefer, easier to understand, and easier to maintain.

Personally, I avoid "using namespace" directives as well, but that is as much a stylistic choice as it is a technical one.

I'm not sure why you're indexing the array backwards in findLowest, but you access array[size], which is out-of-bounds. Should be:

Code:

---

int findLowest (int array[], int size)
{
    int lowest = array[0];
    for (int i = 1; i < size; i++)
        if (array[ i ] < lowest)
            lowest = array[i];
    return lowest;
}

---