-1

So, I have been writing a sorting loop for my program. It automatically sorts Arrays and has done it in the past efficently. That being said, once I added it into my American Football Manager program, things did not work out so effectively.

So below, I will provide a working version of this sorting loop, and then after the Football Manager version which isn't working.

Here is the working one...

#include <string>
using namespace std;
#include <time.h> 
#include <bits/stdc++.h>
#include <windows.h>
#include <conio.h>
#include <stdio.h>



int main()
{
    int i, j,temp;
    int values[10] = { 7, 4, 2, 8, 1, 3, 9, 6, 10, 5 };  // array of 10 'int' values

    int values_count = sizeof(values) / sizeof(int); // 'sizeof' returns the number of 'int' values in the array (10)
cout << values_count;
    printf ("Unsorted...\n");

    for (i = 0; i < values_count; i++)
        printf("element %2d: %2d\n", i, values[i]);

    for (i = 0; i < values_count - 1; i++) // loop from 0 to 8 (the first element is values[0]; 10th is values[9])
    {
        // Compare values[i] with each element from values[i+1] to the values[9] (indexed in the inner loop by 'j').
        // After executing the inner loop, values[i]is now assured to be less than any from values[i+1] to values[9]. 

        for (j = i + 1; j < values_count; j++) // When i=0, loop from 1-9; When i=1, loop from 2-9; When i=2, loop from 3-9, etc.
        {
            // We are sorting to get 'least-to-greatest'. So, any 'values' array elememnt values[j] beyond values[i] 
            // (outer loop) that is greater than values[i] is swapped with the [i] postition in the array

            if (values[j] < values[i])
            {
                temp = values[i];
                values[i] = values[j];
                values[j] = temp;
            }
        }
    }

    printf("\nSorted...\n");

    for (i = 0; i < values_count; i++)
        printf("element %2d: %2d\n", i, values[i]);

    return 0;
}

That one works perfectly. Here is the second that is failing...

#include <string>
using namespace std;
#include <time.h> 
#include <bits/stdc++.h>
#include <windows.h>
#include <conio.h>
#include <stdio.h>



int main()
{
class Team{
public:
    int wins, losses;
    
};



Team teams[10];
int team_count = 10;
int j, i, temp;

teams[1].wins = 1;
teams[2].wins = 2;
teams[3].wins = 5;
teams[4].wins = 3;
teams[5].wins = 6;
teams[6].wins = 10;
teams[7].wins = 7;
teams[8].wins = 4;
teams[9].wins = 8;
teams[10].wins = 9;

for (i = 0; i < team_count; i++){

    printf("element %2d: %2d\n", i, teams[i].wins);

    for (i = 0; i < team_count - 1; i++) // loop from 0 to 8 (the first element is values[0]; 10th is values[9])
    {
        // Compare values[i] with each element from teams[i+1] to the teams[9] (indexed in the inner loop by 'j').
        // After executing the inner loop, teams[i]is now assured to be less than any from teams[i+1] to teams[9]. 

        for (j = i + 1; j < team_count; j++) // When i=0, loop from 1-9; When i=1, loop from 2-9; When i=2, loop from 3-9, etc.
        {
            // We are sorting to get 'least-to-greatest'. So, any 'teams' array elememnt teams[j] beyond teams[i] 
            // (outer loop) that is greater than teams[i] is swapped with the [i] postition in the array

            if (teams[j].wins < teams[i].wins)
            {
                temp = teams[i].wins;
                teams[i].wins = teams[j].wins;
                teams[j].wins = temp;
            }
        }
    }
}









}

This is the output I get for the second version...

element 0: 4253632


Process exited after 0.08073 seconds with return value 0 Press any key to continue . . .

Help appreciated

Troy Cox
  • 5
  • 1
  • 1
    You didn't initialize `teams[0].wins` and you did out-of-bounds write `teams[10].wins = 9;`. – MikeCAT Oct 24 '20 at 01:36
  • Also the inner loop modifies `i` and it is preventing the outer loop from working well. – MikeCAT Oct 24 '20 at 01:37
  • For `Team teams[10];` valid array indicies are 0 .. 9 not 1 .. 10. – drescherjm Oct 24 '20 at 01:38
  • Compile with `-fsanitize=address -fsanitize=undefined` and the compiler will tell your what to fix. – bitmask Oct 24 '20 at 01:38
  • Thanks for catching my newbie mistake xD. Though, even with this change it doesn't work, it only outputs the first element. – Troy Cox Oct 24 '20 at 01:39
  • Shouldn't `teams[x].losses` be sorted according to wins? – MikeCAT Oct 24 '20 at 01:39
  • If it is the inner loop modifying i, it worked on the first version, and not my second? – Troy Cox Oct 24 '20 at 01:40
  • There are no nested loops that use the same loop variable in the first version. There are one in the second version. In the other words, there are no inner loop that modify `i` in the first version. – MikeCAT Oct 24 '20 at 01:40
  • You are printing the wins in the wrong place – drescherjm Oct 24 '20 at 01:41
  • 1
    Your first and second loop try to iterate over the same integer `i`. Hence the first will not run the expected number of times. – bitmask Oct 24 '20 at 01:41
  • how could I change this? Sorry, I'm pretty new to C++ – Troy Cox Oct 24 '20 at 01:42
  • @TroyCox Have a look at [reliable resources for learning C++](https://stackoverflow.com/q/388242/430766) and throw out whatever you are using right now. Browsing the [`[C++-faq]`](https://stackoverflow.com/questions/tagged/c%2b%2b-faq) tag here on SO might also be helpful. – bitmask Oct 24 '20 at 01:45
  • 1
    Unrelated: This odious bit of code `#include ` [should never be directly included in a program](https://stackoverflow.com/questions/31816095/why-should-i-not-include-bits-stdc-h), and the way it's being used demonstrates that you don't properly understand what it is supposed to do. Be very careful when using stuff without understanding how to use it. C++ is an exceptionally unforgiving language. – user4581301 Oct 24 '20 at 01:47
  • Well I ain't gonna throw out my granddad xD. I just started programming and he is a software engineer for HP so he teaches me. Is there a simple way I can fix this problem? – Troy Cox Oct 24 '20 at 01:48
  • Don't throw out your granddad :) But keep in mind that a good engineer is not necessarily a good teacher. Regarding fixing the issue, you have been given two proper answers that address your issue. – bitmask Oct 24 '20 at 01:51
  • 1
    Also note that C++ is a pretty fast-moving language. Stuff is betting added, mostly for the better, all the time. For example, as of the 2017 revision `int values_count = sizeof(values) / sizeof(int);` could be `int values_count = std::size(values);` and since `values` is an array `std::size` should be a compile time constant, so there's no need to cache it in a variable. You can call `std::size(values)` as you need it with no performance hit – user4581301 Oct 24 '20 at 01:56

3 Answers3

1

Here's a much simpler way:

#include <algorithm>

std::sort(std::begin(teams), std::end(teams),
    [](const Team& left, const Team& right) {
        return left.wins < right.wins;
    });
John Zwinck
  • 239,568
  • 38
  • 324
  • 436
0

Here are problems of the second code:

  • Non-standard unused headers are included and they are preventing the code from working in some environments.
  • teams[0].wins is used without being initialized.
  • teams[10].wins = 9; cause dangerous out-of-range access.
  • The sorting routine is accidentally put in the printing routine.
  • Only wins are sorted, not losses, and it will break the information.

Try this:

#include <stdio.h>

int main()
{
    class Team{
    public:
        int wins, losses;
        
    };

    Team teams[10];
    int team_count = 10;
    int j, i;

    teams[0].wins = 1;
    teams[1].wins = 2;
    teams[2].wins = 5;
    teams[3].wins = 3;
    teams[4].wins = 6;
    teams[5].wins = 10;
    teams[6].wins = 7;
    teams[7].wins = 4;
    teams[8].wins = 8;
    teams[9].wins = 9;

    for (i = 0; i < team_count; i++){
        printf("element %2d: %2d\n", i, teams[i].wins);
    }

    for (i = 0; i < team_count - 1; i++) // loop from 0 to 8 (the first element is values[0]; 10th is values[9])
    {
        // Compare values[i] with each element from teams[i+1] to the teams[9] (indexed in the inner loop by 'j').
        // After executing the inner loop, teams[i]is now assured to be less than any from teams[i+1] to teams[9]. 

        for (j = i + 1; j < team_count; j++) // When i=0, loop from 1-9; When i=1, loop from 2-9; When i=2, loop from 3-9, etc.
        {
            // We are sorting to get 'least-to-greatest'. So, any 'teams' array elememnt teams[j] beyond teams[i] 
            // (outer loop) that is greater than teams[i] is swapped with the [i] postition in the array

            if (teams[j].wins < teams[i].wins)
            {
                Team temp = teams[i];
                teams[i] = teams[j];
                teams[j] = temp;
            }
        }
    }

    for (i = 0; i < team_count; i++){
        printf("element %2d: %2d\n", i, teams[i].wins);
    }

}
MikeCAT
  • 73,922
  • 11
  • 45
  • 70
  • While you're at it, you could pull the iteration variable inside the loop-scope (and fix their type) :) – bitmask Oct 24 '20 at 01:48
  • @bitmask Fix their type? How? – MikeCAT Oct 24 '20 at 01:49
  • `int` is a rubbish type for an index variable. Although 10 is guaranteed to fit into every platform's `int`, it is a bad habit and it **will** break one's neck eventually. The index type for sequence containers (such as arrays) is `std::size_t`, not `int`. – bitmask Oct 24 '20 at 01:53
  • One more thing, how can I call like the top two numbers? Say the top two teams play a championship game, I want the computer to know which teams are the top two. How could I go about doing this? – Troy Cox Oct 24 '20 at 02:07
  • The array is sorted in ascending order, so take the last two elements (`teams[team_count - 1]` and `teams[team_count - 2]`). – MikeCAT Oct 24 '20 at 02:10
0

C++20 solution (untested code)

ranges::sort(std::begin(teams), std::end(teams), &Team::wins);

I have waited so long for this ... this might even work

ranges::sort(teams, &Team::wins);
Surt
  • 15,501
  • 3
  • 23
  • 39