give feedback on this pointer program
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.
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
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).
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;
}
If x
is 0, then Average will generate a divide-by-zero error.
Here are some "code review" comments:
In main():
- Change SIZE to "
size_
t" instead ofint
- Why SIZE is uppercase? (May be the author's convention is to have constants as uppercase, in that case it is fine.)
- Combine temp declaration and assignment into one statement as:
double * temp = array;
- What if
inFile
is not available or can't be opened for reading? - What if
inFile
have less than SIZE number of items? - Change the loop variable
i
tosize_t
. - Remove blank line before declaration of
inFile
. - Return some number (e.g.
0
) frommain()
. - Correct the allocation of array.
In Average():
- Change the second argument of Average to
size_t
. - Assert and/or guard for pointer being non-NULL
- 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.
精彩评论