C++ bughunt - High-score insertion in a vector crashes the program

Question

Hello, everyone!

I have a game I'm working on. My players are stored in a vector, and, at the end of the game, the game crashes when trying to insert the high-scores in the correct positions.

Here's what I have (please ignore the portuguese comments, the code is pretty straightforward :P):

//TOTAL_HIGHSCORES is the max. number of hiscores that i'm willing to store. This is set as 10.
bool Game::updateHiScores()
{
    bool stopIterating;
    bool scoresChanged = false;
    //Se ainda nao existirem TOTAL_HISCORES melhores pontuacoes ou se a pontuacao for melhor que uma das existentes
    for (size_t i = 0; i < players.size(); ++i)
    {

    if (hiScores.empty())
        checkForValues = true;
    else
        checkForValues = (hiScores.size() < TOTAL_HISCORES || hiScores.back() < players[i].getScore());
    if (players[i].getScoreValue() > 0  && checkForValues)
        {
            scoresChanged = true;

            if(hiScores.empty() || hiScores.back() >= players[i].getScore())
                hiScores.push_back(players[i].getScore());
            else
            {   
                //Ciclo que encontra e insere a pontuacao no lugar desejado
                stopIterating = false;
                for(vector<Score>::iterator it = hiScores.begin(); it < hiScores.end() && !(stopIterating); ++it) 
                {
                    if(*it <= players[i].getScore())
                    {
                        //E inserida na posicao 'it' o Score correspondente
                        hiScores.insert(it, players[i].getScore());
                        //Verifica se o comprimento do vector esta dentro do desejado, se nao estiver, este e rectificado
                        if (hiScores.size() > TOTAL_HISCORES)
                            hiScores.pop_back();
                        stopIterating = true;
                    }
                }
            }
        }
    }

    if (scoresChanged)
        sort(hiScores.begin(), hiScores.end(), higher);

    return scoresChanged;
}

What am I doing wrong here?

Thanks for your time, fellas.

EDIT:

I ended up simplifying my code to this:

bool scoresChanged;

vector<Score> hiScoresCopy = hiScores;

for (size_t i = 0; i < TOTAL_PLAYERS; ++i)
    hiScoresCopy.push_back(players[i].getScore());

sort(hiScoresCopy.begin(), hiScoresCopy.end(), higher);

scoresChanged = (hiScores != hiScoresCopy);

if (scoresChanged)
{
    if (hiScoresCopy.size() > TOTAL_HISCORES)
        hiScoresCopy.resize(TOTAL_HISCORES);
    hiScores = hiScoresCopy;
}

return scoresChanged;

Answer 1

A couple of things:

• This code looks wrong:

  it < hiScores.end()
  

  because when using iterators and end() you should compare with "!=" instead of "<". You want to iterate until the LAST item, and "!= hiScores.end()" is the way to do that.

• Do you have a debugger available? If so, run your code through it, then see where it dies - that's a fast way to figure out where it's failing.

Answer 2

In your first if, you call hiScores.back() without checking if hiScores is empty.

Answer 3

You don't say exactly where the code crashes or what the nature of the crash so I shall guess.

This test is the wrong way around.

it < hiScores.end() && !(stopIterating)

In one common case your iterator it will be invalidated in the same clause that you set stopIterating to true.

You could do this (!= is more idiomatic than < for iterators although both work for vectors).

!stopIterating && it != hiScores.end()

Alternatively, after your insertion you could just have break; statement and do away with the stopIterating variable. I think that this would be clearer for most readers of the code.

Answer 4

Is there any reason you can't simplify your code to the following?

bool Game::updateHiScores()
{
    for (size_t i = 0; i < players.size(); ++i)
    {
        hiScores.push_back(players[i].getScore());
    }
    sort(hiScores.begin(), hiScores.end(), higher);

    if (hiScores.size() > TOTAL_HIGHSCORES)
    {
        hiScores.erase(hiScores.begin() + TOTAL_HIGHSCORES, hiScores.end());
    }

    return true;
}

Answer 5

I think Charles Bailey is right, but I also think that this code is unnecessarily complex. You could just do this:

bool Game::updateHiScores()
{
  if(players.empty())
    return(false);

  vector<Score> newScores;
  for(vector<Player>::iterator itr = players.begin() ; itr != players.end(); ++itr)
    newScores.push_back(itr->getScore());

  sort(newScores.begin(), newScores.end(), higher);

  if(hiScores.size() < TOTAL_HISCORES || newScores.front() > hiScores.back())
  {
    hiScores.insert(highScores.end(), newScores.begin(), newScores.end();
    sort(hiScores.begin(), hiScores.end(), higher);
    if(hiScores.size() > TOTAL_HISCORES)
      hiScores.resize(TOTAL_HISCORES);
    return(true);
  }
  return(false);
}