2d push_back doesnt save to vector values

Question

I have a 2d vector which should save x and y coordinates. Everything works as intended except saving this values to vector. What did I do wrong?

void Game::GetShips(Board &b)
{
    vector<vector<int>> shipCors;

    for (int i = 0; i < BOARDSIZE; i++) {

        for (int j = 0; j < BOARDSIZE; j++) {
            if (b.getSpaceValue(i, j) == SHIP) {
                cout << "X :"<< i<<"\n Y:"<<j<<'\n';
                shipCors[i].push_back(j);
              
            }
        }

    }
    cout<< shipCors.size()<<'\n';
 }

The outer vector has size 0, so `shipCors[i]` is always undefined behavior. Your `push_back`s don't increase the size because you apply them to the inner vectors (and because UB), not the outer one. — HolyBlackCat, Jul 14 '22 at 09:03
You need to resize the vector for the number of rows you have or `shipCors[i]` is accessing out of bounds. You could also push back a new row in the outer loop if you wanted to do it that way. — Retired Ninja, Jul 14 '22 at 09:03
Maybe a better option would be a `std::vector>`? And if coordinates cannot be negative, I'd use `unsigned int` instead to reflect that fact. — Aconcagua, Jul 14 '22 at 09:07
"your inner vector should be a pointer to a vector" why?!? No pointer needed here — 463035818_is_not_an_ai, Jul 14 '22 at 09:09
@IlanKeshet thats non-sence. `operator[]` returns a reference. push_back would work fine if `shipCors[i]` would actaully be a vector, but it isnt because `shipCors` is empty — 463035818_is_not_an_ai, Jul 14 '22 at 09:20
the proposed dupe was a bout confusing a global with a local variable. I am certain there are good dupes but that wasnt one — 463035818_is_not_an_ai, Jul 14 '22 at 09:32

Vlad from Moscow · Accepted Answer · 2022-07-14T12:53:23.117

1

You declared an empty vector

vector<vector<int>> shipCors;

So you may not use the subscript operator

shipCors[i].push_back(j);

You could write

for (int i = 0; i < BOARDSIZE; i++) {

    shipCors.resize( shipCors.size() + 1 );
    for (int j = 0; j < BOARDSIZE; j++) {
        if (b.getSpaceValue(i, j) == SHIP) {
            cout << "X :"<< i<<"\n Y:"<<j<<'\n';
            shipCors[i].push_back(j);
          
        }
    }

}

Pay attention to that as you are using the index i then you need to add a "row" of the vector even if the row will be empty after executing the inner for loop.

It will be even better to resize the vector initially before the for loops like

vector<vector<int>> shipCors;
shipCors.resize( BOARDSIZE );

for (int i = 0; i < BOARDSIZE; i++) {

    for (int j = 0; j < BOARDSIZE; j++) {
        if (b.getSpaceValue(i, j) == SHIP) {
            cout << "X :"<< i<<"\n Y:"<<j<<'\n';
            shipCors[i].push_back(j);
          
        }
    }

}

An alternative approach is to have a vector declared like

std::vector<std::pair<int, int>> shipCors;

In this case your loop will look like

for (int i = 0; i < BOARDSIZE; i++) {

    for (int j = 0; j < BOARDSIZE; j++) {
        if (b.getSpaceValue(i, j) == SHIP) {
            cout << "X :"<< i<<"\n Y:"<<j<<'\n';
            shipCors.emplace_back(i, j);
          
        }
    }

}

Or to keep the data sorted you can declare a set like

std::set<std::pair<int, int>> shipCors;

edited Jul 14 '22 at 12:53

answered Jul 14 '22 at 09:16

Vlad from Moscow

301,070
26
186
335

`shipCors.resize(i)` is shorter and `if ( i >= shipCors.size())` a little safer – 463035818_is_not_an_ai Jul 14 '22 at 09:22
1

@463035818_is_not_a_number It is in general an incorrect approach. You need to add a new "row" in any case. I have updated my post.:) – Vlad from Moscow Jul 14 '22 at 09:28
`for (int i = 0; i < BOARDSIZE; i++) { shipCors.resize( shipCors.size() + 1 );` – **honestly???** In worst case you produce a bunch of totally needless re-allocations while `BOARDSIZE` is fix anyway! *Please* do `shipCors.resize(BOARDSIZE)` *before* the loop or alternatively provide appropriate size already to constructor (`vector> shipCors(BOARDSIZE);`). – Aconcagua Jul 14 '22 at 11:52
@Aconcagua It can be done before loops but I think the approach with the vector is not good. As I pointed to it is better to use std::set. – Vlad from Moscow Jul 14 '22 at 12:51
@VladfromMoscow Indeed, agree on, proposed a 1D vector of pairs myself, too, in comments to question ;) Still that code appears – let's say – pretty questionable and I wouldn't show up in an answer for a beginner who might adopt it later on... Apart from, simply calling `emplace_back` might have been more elegant. – Aconcagua Jul 14 '22 at 19:48

2d push_back doesnt save to vector values

1 Answers1