Skip to content

Instantly share code, notes, and snippets.

@noxee

noxee/review.md Secret

Last active March 30, 2016 18:28
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save noxee/69e43e39e9e04e716f8739efcb333c36 to your computer and use it in GitHub Desktop.
Save noxee/69e43e39e9e04e716f8739efcb333c36 to your computer and use it in GitHub Desktop.
DeviantRouge code review

My first recommendation would be if you're allowed to, to use vectors. The reason being is that it will handle the allocation of memory as you add items to the vector. Currently you are creating an array with a size of 30 and it looks like your code assumes that it may not read the max number of the values (as indicated by passing in nDays). So if you only read in 15 values you've over allocated by 15. It's not a major issue but it's something to consider.

The other advantage of a vector is that you make a call like temps.size() which will return the number of elements in the vector (rather than tracking how many values you have).

Another general note is that all arrays are zero indexed. So when you are printing your values you're going to have to add 1 to the index value otherwise you'll be printing out something like June 0.

ifstream inFile;
inFile.open(fileName.c_str());

Can be written as:

ifstream ifile(fileName, ios::in);

This also tells the runtime environment that you are only performing input operations on this file.

Whenever you open a file you should check that the file successfully opened before you perform any additional operations on it.

Something like this should suffice:

if (!ifile.is_open()) {
  std::cerr << "There was a problem opening the input file!\n";
  exit(1);
}
temperature;

That line is going to conflict with the passed in array temperature[]. You should have a separate variable (with a different name) for storing value read from the file.

With those changes your function could look something like:

void getData(string fileName, double temperature[], int &nDays)
{
  nDays = 0;
  double temp = 0.0;

  ifstream ifile(fileName, ios::in);
  
  while (ifile >> temp)
  {
      cout << temp << " and June " << nDays << endl;
      temperature[nDays] = temp;
      nDays++;
  }

    cout <<"in getData ... numDays is " << nDays << endl;
}
double findHighestTemp(double temperature[], int numDays, int highest)
{

    for (int i = 1; i > numDays; i++)
    {

        if (temperature[i] < temperature[higest])
        int higest = i;
    }

    return highest;

}

This function is a bit of a mess and I think a couple of other people have given you pointers. But ideally the purpose of this function would be to return the index of the highest temperature. So you wouldn't be passing in int highest as it's no longer needed. (You will still need to track your highest temperature for comparison purposes).

Also as pointed out for (int i = 1; i > numDays; i++) you're going to miss the first element but staring at 1.

So as a starting point for this function you could start with the following function signature:

int findHighestTemp(double temperature[], int numDays)

The function would then do the following:

  • Set initial value of highest temperature to first element of temperature array.
  • Set highest index to 0.
  • Iterate over array (starting index at 0) for all values in temperature array.
  • Check if current value of array is greater than current highest temp. If so store index in highest index variable.
  • After iterating over all elements return highest index.
int showDaysOver100(double temperature[], int numDays, int tempOver100)
{
    for (int i = 0; i < numDays; i++)
    {
        if (temperature[i] => tempOver100)
        {
            int dayOver100 = 0;
            dayover100++;
        }


    }
}

To make this function more generic I would start with the following function signature:

int daysOverTemp(double temperature[], int numDays, int checkTemp)

Then in the function I would do the following:

  • Create daysOverTemp variable and initialize it to 0.
  • Iterate over all temperature values. Check if current temp is over checkTemp. If so increment daysOverTemp variable.
  • Return daysOverTemp
double averageTemp(double temperature[], int numDays)
{
    for (i = 0; i < numdays; i++)
    {
        totalTemps = 0;

        totaltemps += temperature[i];
    }

    double average = totalTemps / numDays;

    return average;
}

This function is almost there but you are declaring your variable in the wrong spot (or not declaring them properly at all).

You want to do the following:

  • Define and initialize variable sum to 0.
  • Iterate over all temperatures and add their value to sum.
  • Return sum divided by numDays

You should be able to sort out the main if you pay attention to the new function signatures and their return values.

One thing to note. You should probably change const string fileName = "summer_temps.txt"; to be the full file path and not just the relative path.

@noxee
Copy link
Author

noxee commented Mar 30, 2016

Try making your program work with the current changes.

After that if you'd like to see a working program that makes use of vectors you can check out this.

@noxee
Copy link
Author

noxee commented Mar 30, 2016

Once you've got a working version of your own code feel free to check out this which is my version of your code using arrays (as opposed to vectors).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment