Pancake Glutton Exercise - How can I improve it?

[Arslan7041]

So I did the pancake glutton exercise which you can find here: http://www.cplusplus.com/forum/articles/12974/

My code for it is below.

It works, but Im just looking for some comments and criticisms on it. How can I optimize it? Any suggestions will be appreciated.

One question in particular I had in mind: Is this possible to do with only a single array?

Note: Dont bother about the bubble sort; I know its not the most efficient sorting algorithm. I'll replace that later with a different sorting algorithm.

#include <iostream>

using namespace std;

//Prototypes
int get_Max(int[], int);
int get_Min(int[], int);
void BubbleSort(int[], int[], int);
void PrintList(int[], int[], int);

int main()
{
    int person[10], // stores persons
        numPancakes[10]; // stores number of pancakes

    // Fill up person array with person #'s
    for(int i=0; i<=10; i++)
    {
        person[i] = i+1;
    }

    // Fill up numPancakes array with user input
    for(int i=0; i<10; i++)
    {
        cout << "Enter number of panacakes eaten by person " << person[i] << ": ";
        cin >> numPancakes[i];
    }


    int maxPerson = get_Max(numPancakes, 10); // call get_Max function
    cout << "\nMost number of pancakes were eaten by person " << maxPerson << endl;

    int minPerson = get_Min(numPancakes, 10); // call get_Min function
    cout << "Least numer of pancakes were eaten by person " << minPerson << endl;

    cout << "\n";

    BubbleSort(person, numPancakes, 10); // Call Bubble Sort function

    PrintList(person, numPancakes, 10); // Call PrinList function


   cin.get();
}

int get_Max(int numPancakes[], int elements)
{
    int maxPerson = 1; // default is person 1 in case the if-condition is never true
    int temp = numPancakes[0]; // temp stores the number of pancakes, not the person #
    int i; // loop counter

    for(i=0; i<elements; i++)
        if(numPancakes[i]>temp)
        {
            temp = numPancakes[i];
            maxPerson = i+1; // Stores person # (exactly what we want) into maxPerson
        }

    return maxPerson;
}

int get_Min(int numPancakes[], int elements)
{
    int minPerson = 1; // default is person 1 in case the if-condition in never true
    int temp = numPancakes[0]; // temp stores number of pancakes, not person #
    int i; // Loop counter

    for(i=0; i<elements; i++)
        if(numPancakes[i]<temp)
        {
            temp = numPancakes[i];
            minPerson = i+1; // Stores person # into minPerson
        }


    return minPerson;

}

void BubbleSort(int person[], int numPancakes[], int elements)
{
    int temp1, temp2;

    // Bubble Sort
    for(int pass=0; pass<elements; pass++)
        for(int i=0; i<elements-1; i++)
                if(numPancakes[i]<numPancakes[i+1])
                {
                    // Swap numPancakes[i] and numPancakes[i+1]
                    temp1 = numPancakes[i];
                    numPancakes[i] = numPancakes[i+1];
                    numPancakes[i+1] = temp1;

                    // Swap person[i] and person [i+1]
                    temp2 = person[i];
                    person[i] = person[i+1];
                    person[i+1] = temp2;
                }
    return;
}

// Simply prints the list
void PrintList(int person[], int numPancakes[], int elements)
{
    for(int i=0; i<elements; i++)
    {
        cout << "Person " << person[i] << ": ate " << numPancakes[i] << " pancakes" << endl;
    }
}

[MiiNiPaa]

1) You have an error in snippet:

    for(int i=0; i<=10; i++) {
        person[i] = i+1;
    }

There is no person[10], and you are trying to access it.

2) Do not reinvent the wheel. There is std::sort, std::max_element, std::min_element (and even std::minmax_element) in standard library
All of your auxilary functions are not needed.

3) Notice that you do not strictly need to use get_XXX functions at all. After sort, max element would be first and least would be last.

4) That "10" denoting number of persons should be a constant in your program, not a literal sprinkled everywhere.

Just to compare, here is my quick and dirty solution (uses C++14):

1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28

#include <algorithm> //for std::sort #include <array> //for std::array #include <functional>//for std::greater #include <iomanip> //for std::setw #include <iostream> #include <utility> //for std::pair int main() { constexpr size_t persons = 10; std::array<std::pair<int, int>, persons> eaten; for(int i = 1; i < 11; ++i) { std::cout << "Number of pancakes person " << i << " have eaten: "; int temp; std::cin >> temp; eaten[i-1] = std::make_pair(temp, i); } std::sort(eaten.begin(), eaten.end(), std::greater<>()); std::cout << "\nMost pancakes were eaten by person " << eaten.front().second << '\n'; std::cout << "\nLeast pancakes were eaten by person " << eaten.back().second << '\n'; for(int i = 0; i < 10; ++i) { std::cout << "Person " << std::setw(2) << eaten[i].second << " have eaten " << eaten[i].first << " pancakes\n"; } }

Number of pancakes person 1 have eaten: 10 Number of pancakes person 2 have eaten: 2 Number of pancakes person 3 have eaten: 14 Number of pancakes person 4 have eaten: 8 Number of pancakes person 5 have eaten: 12 Number of pancakes person 6 have eaten: 4 Number of pancakes person 7 have eaten: 11 Number of pancakes person 8 have eaten: 9 Number of pancakes person 9 have eaten: 6 Number of pancakes person 10 have eaten: 3 Most pancakes were eaten by person 3 Least pancakes were eaten by person 2 Person 3 have eaten 14 pancakes Person 5 have eaten 12 pancakes Person 7 have eaten 11 pancakes Person 1 have eaten 10 pancakes Person 8 have eaten 9 pancakes Person 4 have eaten 8 pancakes Person 9 have eaten 6 pancakes Person 6 have eaten 4 pancakes Person 10 have eaten 3 pancakes Person 2 have eaten 2 pancakes

http://coliru.stacked-crooked.com/a/ecf73654b916cc2b

[Arslan7041]

(1) Woops! Can't believe I missed that. I will fix that soon.

(2) Sorry, im just a beginner. Im not aware of all those functions, and I dont really know how to use them (though I plan on learning). But even so, as a beginner I think its better that I write these 'unnecessary" auxiliary functions rather than take the 'shortcuts' and throw the array into an library function. When I get more advanced, I will start using those other libraries.

(3) Good point. Thank you.

(4) Again, stupid mistake on my part. I've always learned to make these types of numbers constants. Dont know why I didn't do that here. Will change that.

Looking at your quick and dirty solution, I am very confused as to what is going on. I dont even understand how your array declaration works nor what 'pair' does. But thats fine because I plan on learning all these things in the near future.

Thank you.

[Arslan7041]

I've made the changes. Updated code is below:

#include <iostream>

using namespace std;

//Prototypes
void BubbleSort(int[], int[], int);
void PrintList(int[], int[], int);

int main()
{
    const int N = 10;

    int person[N], // stores persons
        numPancakes[N]; // stores number of pancakes

    // Fill up person array with person #'s
    for(int i=0; i<N; i++)
    {
        person[i] = i+1;
    }

    // Fill up numPancakes array with user input
    for(int i=0; i<N; i++)
    {
        cout << "Enter number of panacakes eaten by person " << person[i] << ": ";
        cin >> numPancakes[i];
    }

    cout << "\n";

    BubbleSort(person, numPancakes, N); // Call Bubble Sort function

    cout << "Most number of pancakes were eaten by person " << person[0];
    cout << "\nLeast number of pancakes were eaten by person " << person[N-1];
    
    cout << "\n\n";

    PrintList(person, numPancakes, N); // Call PrintList function


   cin.get();
}

void BubbleSort(int person[], int numPancakes[], int elements)
{
    int temp1, temp2;

    // Bubble Sort
    for(int pass=0; pass<elements; pass++)
        for(int i=0; i<elements-1; i++)
                if(numPancakes[i]<numPancakes[i+1])
                {
                    swap (numPancakes[i], numPancakes[i+1]);
                    swap (person[i], person [i+1]);
                }
    return;
}

// Simply prints the list
void PrintList(int person[], int numPancakes[], int elements)
{
    for(int i=0; i<elements; i++)
    {
        cout << "Person " << person[i] << ": ate " << numPancakes[i] << " pancakes" << endl;
    }
}

Any other suggestions/comments/criticisms are welcomed.

Notice that I've replaced by 3 assignment statement swaps in the bubble sort with the swap functions, which I am somewhat familiar with. I am not, however, sure how their inner mechanics work. Does the swap function use the same 3 assignment statements to swap two things or is it a different algorithm?

My initial question is still unanswered, however: Is there a way to do this problem using a single array?

@MiiNiPaa: Looking at your quick and dirty solution, it looks like you used some type of two-dimensional array in your code? Is that correct? So Im assuming a single one-dimensional array would not be sufficient for this type of problem, otherwise you probably would have used that. Please comment on this.

[48T7M4Gy]

Can't you eliminate the numPancakes array just by referring to the person by their index number in the person array? You're only required to store the number rather than name etc.

[MiiNiPaa]

Does the swap function use the same 3 assignment statements to swap two things or is it a different algorithm?

It is assumed to be optimised for built-in types. Usually it uses move-copying/move-assigment.

Is there a way to do this problem using a single array?

Yes. I forgot to mention in my initial answer, but parallel arrays are error-prone.

If you have data which should be held together, group it together:

//Using classes:
struct Person
{
    int eaten;
    int number;
};
//...
Person participants[10];

//Grouping it in pair
//We need to rememeber that first is nmber of pancakes eaten and second is position number
std::pair<int, int> participants[10];

I chose pair for my code (and I have a C++ array of pairs because it is already has defined comparison operators and compatible with std::sort without any additional work. This is why number of pancakes is first entry in pair: we need to sort by number of pancakes first.

[48T7M4Gy]

#include <iostream>
using namespace std;

int main()
{
    const int N = 10;
    int person[N] = {1, 3, 7, 10, 5, 0, 0, 8, 2, 1}; // stores number of cakes
    
    int least = person[0], leastIndex = 0;
    int  most = person[0],  mostIndex = 0;
    
    for (int i = 0; i < N; i++)
   {
       if  (person[i] < least)
       {
           least = person[i];
           leastIndex = i;
       }
       
       if  (person[i] > most)
       {
           most = person[i];
           mostIndex = i;
       }
       
       cout << "Person " << i << ": ate " << person[i] << " pancakes" << endl;
    }
    
    cout << " Most eaten " << most << " by person " << mostIndex << endl;
    cout << "Least eaten " << least << " by person " << leastIndex << endl;
    
}

[Arslan7041]

@kemort: Your code displays the list by person # order. The problem requires the list to be displayed by the number of pancakes (from greatest eaten to least eaten).

[48T7M4Gy]

@arslan - you are right but I knew that because there is still the sort to do.

I am demonstrating that only one array is necessary provide the number eaten is the only data required to be stored, as I said.

You can apply that array to your bubblesort function if you modify the function. And, if you do that my least/most calculation can be dumped too as you commented with MiiniPaa.