Help with proper programming practices

[thechad90000]

I just did this program for Project Euler 8 and it works fine first and foremost. I was supposed to read in from a file a 1000 digit number and find the largest product of any 5 consecutive numbers.

As I said the program works fine but what I'm worried about it proper programming practices. Aside from obviously no comments, what advice could you give me about making this more streamlined and acceptable when it comes to speed and proper practice. Thank you

#include <iostream>
#include <cstdlib>
#include <fstream>


using namespace std;

int main ()
{
  int longest = 0;
  int current = 0;
  char c [999];
  int x [999];
  ifstream file;
  file.open ("example.txt");
  
  if (!file)
  {
    cout << "File did not open correctly" << endl;
    return 0;
  }
  
  for (int i = 0; file.good(); i++)
  {
    c[i] = file.get();
  }
  
  for (int i = 0; i <=999; i++)
  {
    x[i] = c[i] - '0';
  }
  
  for (int i = 0; i < 995; i++)
  {
    current = x[i] * x[i+1] * x[i+2] * x[i+3] * x[i+4];
    
    if (current > longest)
      longest = current;
  }
  
  cout << longest << endl;
  
      
  file.close();
  
  
  return 0;
}

[ne555]

for (int i = 0; i <=999; i++)is out of bounds,
You want to read a 1000 digit number, but your array only has 999 elements.

_ RAII: open your file with the constructor, close it with the destructor. ifstream file("example.txt");
_ Send error messages to the error stream std::cerr
_ Don't loop on good() or eof(). Loop on the reading (or in this case, the number of elements).
_ Don't use magic numbers (999, 995)
_ Try to generalize. Create functions, that use parameters.

[Veltas]

Are there any zeros in the list?

If not, then an improvement would be to multiply the first 5 only, and then divide by the previous first of five and multiply by the new last each time.

e.g. With the list 7 3 6 7 3 9 you would perform:

currentMax = a = 7 * 3 * 6 * 7 * 3

currentMax = max ( currentMax, a * 9 / 7 )

rather than:

currentMax = max ( currentMax, 3 * 6 * 7 * 3 * 9 )

Also, you're doing unnecessary loops. A char is merely another integer that encodes different ASCII characters, so there's no danger or ambiguity in the replacement loop:

for (int i = 0; file.good(); i++)
{
  x[i] = file.get() - '0';
}

Honestly, I'm surprised the program isn't crashing, when declare an array you give it the size of the array, not the last index, so get rid of c as it's unnecessary and replace your declaration of x with:

int x[1000];

As you want a size 1000 array, not a size 999 array! 999 is the index of the final element, but not the size.

And close your file right after the loop that gets all the elements, it makes almost no difference with this program but as you want good practice, good practice is closing a file as soon as you're done so that other programs can use it!

[ResidentBiscuit]

for (int i = 0; i < 995; i++)

This leaves the last 5 digits uncalculated. This stops at element 994. Which leaves 995,996,997,998,999 all unaccounted.

As you said, you found the answer anyway, but the possibility of the answer lying in those last 5 is possible.

[Volatile Pulse]

No it didn't resident. He omitted the last five values since in the following statement he did x+4. He did however omit the very last one. It should have been x<=995

[ResidentBiscuit]

Ah you are right. This is what I get for only half reading code.

[thechad90000]

Just wanted to say thanks for all the great input that you guys have helped me with. I'm definitely working on getting all that stuff sorted out.