Program Keeps Crashing

[MottMan]

I made this program for the beginner exercises on this site but it keeps crashing. It may not work 100% yet (if it doesn't I would like to discover that for myself) but I need help to get it to stop crashing. If I input one through ten it works, but if I enter a variety of numbers it crashes. I don't think it's a type-error but I'm not an expert either. Here is the code:

#include <iostream>

int main()
{
    using namespace std;
    
    int NumofPan[10];
    int OrderofPan[10];
    int PersonNum[10];
    int pmax;
    
    int i = 0;
    int j = 0;
    
    for (i = 0; i < 10; i++)
    {
          cout << "How many Pancakes did person " << ++i << " eat?\n";        
          cout << "Number of Pancakes: ";
          --i;
          cin >> NumofPan[i];    
          cout << endl;          
    }
    
    pmax = 0;
           
    for (j = 0; j < 10; j++)
    {
          for (i = 0; i < 10; i++)
          {
                if (pmax < NumofPan[i] && NumofPan[i] > (-1))
                {  
                      pmax = NumofPan[i];
                      PersonNum[j] = i;
                      OrderofPan[j] = NumofPan[i];                                 
                }                
          } 
          i = PersonNum[j];
          NumofPan[i] = (-1); 
          pmax = 0;  
    } 
    
    for (i = 0; i < 10; i++)
    cout << "Person " << ++PersonNum[i] << " ate " << OrderofPan[i] << " pancakes\n";
    
    cin.get();
    cin.get();
    return 0;
}

Any help is greatly appreciated.

EDIT: I know it's a segmentation fault and it's because of the -1. But why? (I actually don't know what a segmentation fault is...)

[filipe]

I just took a quick look, but I'll bet this is your problem:

i = PersonNum[j];

You're probably setting i to some garbage value because PersonNum[j] wasn't initialized, and then when you try to access NumofPan[i] you crash.

[MottMan]

It's because of the:

NumofPan[i] = (-1);

If I change the -1 to a 0 (also changing it in line 30) it works. 0 is a valid number of pancakes though. I would prefer it if it was -1 because nobody can eat minus pancakes. Thanks anyway though.

[filipe]

Actually, when you change line 30, you're probably making sure the condition is always true, so the entire PersonNum array is initialized. It has nothing to do with the value -1 itself.

[MottMan]

But it works if I change it to 0 (granted 0 wasn't one of the numbers entered by the user). Line 38 is setting it so when it loops back to line 30 it will recognize numbers that have already been placed the OrderofPan array. This way it doesn't compare a number more than once. Maybe I just understood what your saying?

[filipe]

Let me be clear: if the condition on line 30 is false for just one j, an element in array PersonNum won't ever be initialized. So, when control leaves the inner loop and assigns i to whatever garbage value is sitting in PersonNum[j]'s memory, i will have a value that is very likely not in the 0 to 9 range. Then you try to access NumofPan[i] for that i, and you try to write (whatever value may be) into it, the program crashes because you can't access that random memory address.

[MottMan]

That makes more sense, thanks. But then why does it work with 0?

[kempofighter]

If this is a beginner exercise then there are significant improvements that must be made. First I counted 7b occurrences of 10 being used. Instead define one constant and then use the constant throughout. lines 17 and 19 are nonsensical. Just use the expression (i + 1) so that you don't have to actually change the value of i multiple times within each loop. You aren't doing any error checking on the cin operations. Therefore if the user accidentally enters a letter instead of a number the array will be left uninitialized. The endl at line 21 needs to be part of the expression on line 18 otherwise there is no guarantee that the user will even see the initial message. At this point you are just getting lucky.

Since it is impossible to eat -x pancakes you probably want something like this.

while( (cout << "How many Pancakes did person " << i + 1 << " eat? \n" << std::endl) && 
         (!(cin >> NumofPan[i]) || (NumofPan[i] < 0)))
{
     std::cout << "That's not a valid number of pancakes! " << std::endl;
     std::cin.clear();
     std::cin.ignore(std::numeric_limits<std::streamsize>::max(), '\n');
}

The second set of for loops which I guess is for sorting is non-sensical. Why would you ever want to overwrite what is inside the array? Just use std::sort. To print in descending order you can reverse iterate or specify std::greater<int> as the predicate for sort.

[kempofighter]

That makes more sense, thanks. But then why does it work with 0?

It doesn't. You shouldn't be overwriting anything within that array with -x, 0, x, y, z, or anything else. The idea is to swap elements in order to reorder them. For that you use the temporary variable so that nothing is lost.
"code doesn't crash" != "code works"