give feedback on this pointer program

Question

This is relatively simple program. But I want to get some feedback about how I can improve this program (if any), for example, unnecessary statements?

#include<iostream>
#include<fstream>
using namespace std;

double Average(double*,int);

int main()
{

    ifstream inFile("data2.txt");

    const int SIZE = 4;
    double *array = new double(SIZE);
    double *temp;
    temp = array;

    for (int i = 0; i < SIZE; i++)
    {
        inFile >> *array++;
    }
    cout << "Average is: " << Average(temp, SIZE) << endl;
}

double Average(double *pointer, int x)
{
    double sum = 0;

    for (int i = 0; i < x; i++)
    {
        sum += *pointer++;
    }
    return (sum/x);
}

The codes are valid and the program is working fine. But I just want to hear what you guys think, since most of you have more experience than I do (well I am only a freshman ... lol)

Thanks.

Answer 1

Fix the memory leak. i.e delete temp; Also, check if the file is open/created..etc

ideally, you should manipulate/traverse the array using your temp variable instead of using *array itself

Answer 2

If x is 0, then Average will generate a divide-by-zero error.

Answer 3

Since we're talking about C++, I would suggest using STL containers and algorithms. I also find that in most cases it's better to use references or smart pointers (e.g. boost::shared_ptr) instead of raw pointers. In this case there's no need for pointers at all.

Here is how you could write your program:

#include <fstream>
#include <vector>
#include <iostream>
#include <numeric>
#include <iterator>

using namespace std;

int main()
{
    ifstream f("doubles.txt");
    istream_iterator<double> start(f), end;
    vector<double> v(start, end);

    if (v.empty())
    {
        cout << "no data" << endl;
        return 0;
    }

    double res = accumulate(v.begin(), v.end(), 0.0);
    cout << "Average: " << res / v.size() << endl;
    return 0;
}

Answer 4

Sorry to post this as an answer - seem to have trouble commenting (or is this intentional? new to the site). In terms of your question 'what do you mean by traversing the array'? That's just another term for looping through an object (an array here). While your code

for (int i = 0; i < SIZE; i++)  
{  
    inFile >> *array++;  
}  

is valid, it's a bit awkward: you're mixing two paradigms, referencing by pointer with referencing by indices. You're counting down to 3, but actually use the pointer. I assume he meant to suggest to instead actually use the 'i' as in

for (int i = 0; i < SIZE; i++)
{
    inFile >> array[i];  
}  

(you could also eliminate the i loop and instead use the pointer progression to steer your loop.).

Answer 5

You are not initializing your array correctly. This statement:

double *array = new double(SIZE);

Allocates one double and initializes it to the value of SIZE. What you should be doing is using array allocation:

double *array = new double[SIZE]; 

Another general problem is you rarely ever want to assign dynamically allocated memory to a raw pointer. If you want to use base types instead of higher level objects such as std::vector, you should always use a smart pointer:

boost::scoped_array<double> array(new double[SIZE]);

Now the array will automatically get freed regardless of how you leave your scope (i.e. from a newly added return or from an exception).

Answer 6

Here are some "code review" comments:

In main():

1. Change SIZE to "size_t" instead of int
2. Why SIZE is uppercase? (May be the author's convention is to have constants as uppercase, in that case it is fine.)
3. Combine temp declaration and assignment into one statement as: double * temp = array;
4. What if inFile is not available or can't be opened for reading?
5. What if inFile have less than SIZE number of items?
6. Change the loop variable i to size_t.
7. Remove blank line before declaration of inFile.
8. Return some number (e.g. 0) from main().
9. Correct the allocation of array.

In Average():

1. Change the second argument of Average to size_t.
2. Assert and/or guard for pointer being non-NULL
3. Assert and/or guard against division by zero.

Acknowledgement: Some points are collected from other answers. I tried to make a complete list as far as I could.