开发者

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():

  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.

0

上一篇:

下一篇:

精彩评论

暂无评论...
验证码 换一张
取 消

最新问答

问答排行榜