CODEWITHSUNDEEP
c++vector
Will Howe
Will Howe

Reputation: 139

Vector going out of bounds

I'm attempting to iterate through a list of 6 'Chess' pieces. Each round they move a random amount and if they land on another then they 'kill' it.

The problem is that when the last piece in my vector kills another piece I'm getting a vector 'out of range' error. I'm guessing it's because I'm iterating through a vector whilst also removing items from it, but I'm not increasing the count when I erase a piece so I'm not entirely sure. Any help would be greatly appreciated.

Here is my vector:

vector<Piece*> pieces;
pieces.push_back(&b);
pieces.push_back(&r);
pieces.push_back(&q);
pieces.push_back(&b2);
pieces.push_back(&r2);
pieces.push_back(&q2);

and this is the loop I iterate using:

while (pieces.size() > 1) {
    cout << "-------------- Round " << round << " --------------" << endl;
    round++;
    cout << pieces.size() << " pieces left" << endl;
    i = 0;
    while (i < pieces.size()) {
        pieces.at(i)->move(board.getMaxLength());
        j = 0;
        while (j < pieces.size()) {
            if (pieces.at(i) != pieces.at(j) && col.detectCollision(pieces.at(i), pieces.at(j))) {
                cout << pieces.at(i)->getName() << " has slain " << pieces.at(j)->getName() << endl << endl;
                pieces.at(i)->setKills(pieces.at(i)->getKills() + 1);
                pieces.erase(pieces.begin() + j);
            }
            else {
                j++;
            }
        }
        i++;
    }
}

Solution

pieces.erase(pieces.begin() + j);
break;

Upvotes: 1

Views: 700

Answers (3)

Yola
Yola

Reputation: 19023

You should save locally pieces.at(i) and use this local variable everywhere you use pieces.at(i).

To avoid both elements out of bound and logical problems you can use std::list.

As aside, you should use std::vector<Piece*> only if these are non-owning pointers, otherwise you should use smart-pointers, probably unique_ptr.

Upvotes: 1

Francis Cugler
Francis Cugler

Reputation: 7905

I have a very strong feeling that this line of code: 

i++;

is your culprit which is missing either a needed break condition or another conditional check that is missing from your loops. As it pertains to your nested while loops' conditions since they are based on the current size of your vector and are not being updated accordingly.

while (pieces.size() > 1) {
    // ...
    while (i < pieces.size()) {
        // ...
        while (j < pieces.size()) {
              // ...
        }
    }
}

This is due to the fact that you are calling this within the inner most nested loop:

 pieces.erase(pieces.begin() + j);

You are inside of a nested while loop and if a certain condition is met you are then erasing the object at this index location within your vector while you are still inside of the inner while loop that you never break from or check to see if the index is still valid.

Initially you are entering this while loop with a vector that has 6 entries, and you call erase on it within the nested loop and now your vector has 5 entries.

This can reek havoc on your loops because your index counters i & j were set according to the original length of your vector with a size of 6, but now the vector has been reduced to a size of 5 while you are still within the inner most nested loop that you never break from nor check to see if the indices are valid. On the next iteration these values are now invalidated as you never break out of the loops to reset the indices according to the new size of your vector, nor check to see if they are valid.

Try running this simple program that will demonstrate what I mean by the indices being invalidated within your nested loops.

int main() {
    std::vector<std::string> words{ "please", "erase", "me" };

    std::cout << "Original Size: " << words.size() << '\n';
    for (auto& s : words)
        std::cout << s << " ";
    std::cout << '\n';

    words.erase(words.begin() + 2);

    std::cout << "New Size: " << words.size() << '\n';
    for (auto& s : words)
        std::cout << s << " ";
    std::cout << '\n';

    return 0;
}

-Output-

Original Size: 3
please erase me
New Size: 2
please erase

Upvotes: 2

zertyz
zertyz

Reputation: 705

Your logic needs a little refinement.

The way you coded it, it seems the "turn based" nature of chess has been replaced by a kind of "priority list" -- the pieces closer to the start of the vector are allowed to move first and, thus, get the priority into smashing other pieces.

I don't know if you want this logic to be right or wrong. Anyway, the trouble seems to be due to unconditionally executing the line

i++;

It should not be executed if you remove a piece for the same reason 'j++' isn't executed: you will jump over a piece.

Upvotes: 3

Related Questions