Have Geek Brain

Give feedback on this index program

This is a relatively simple program. But I want some feedback on how I can improve this program (if any), like 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 works fine. But I just want to hear what you guys think because most of you have more experience than me (well, I'm just a beginner ... lol)

Thanks.

+2


a source to share


5 answers


Fix memory leak. ie remove temp; Also check if /etc..etc file is open



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

+4


a source


You are not initializing your array correctly. This statement:

double *array = new double(SIZE);

Allocates one double and initializes it to SIZE. What you have to do is use an array allocation:

double *array = new double[SIZE];


Another common problem is that you rarely 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]);

The array will now be automatically deallocated regardless of how you leave scope (i.e. from a recently added return or from an exception).

+4


a source


Since we are talking about C ++, I would suggest using STL containers and algorithms. I also found that in most cases it is better to use references or smart pointers (like boost :: shared_ptr) instead of raw pointers. In this case, there is no need for pointers.

This 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;
}
+2


a source


If x

equal to 0, then Average will generate a division by zero error.

+1


a source


Here are some comments on the code review:

In main ():

  • Change SIZE to " size_

    t" insteadint

  • Why is SIZE uppercase? (Maybe the copyright agreement should have the constants uppercase, in which case that's fine.)
  • Combine temp declaration and assignment into one statement as: double * temp = array;

  • What if inFile

    unavailable or cannot be opened for reading?
  • What if the inFile

    number of items is less than SIZE?
  • Change the loop variable i

    to size_t

    .
  • Remove the blank line before the declaration inFile

    .
  • Return some number (for example 0

    ) from main()

    .
  • Correct array selection.

Average():

  • Change the second argument, Average to size_t

    .
  • Asserting and / or protecting a non-null pointer
  • Acknowledgment and / or protection against division by zero.

Confirmation . Some points are collected from other answers. I tried to make the list as complete as I could.

0


a source






More articles:


All Articles