##First Pass:
First Pass:
##First Pass:
First Pass:
-Wall -Wextra -Wshadow -Werror
-Wall -Wextra -Werror
-Wall -Wextra -Wshadow -Werror
##First Pass:
void Board::traverse (Tile *t, Neighbours neighbours)
^^^^ Pointers are bad idea.
They make resource management hard to
reason about as pointers have no
ownership semantics. Prefer a reference
if it can not be nullptr.
void Board::traverse (Tile *t, Neighbours neighbours)
^^^^^^^^^^^^^^^^^^^^^
Here you are passing neighbours by value.
This means you are making a copy of the
object. Prefer const reference.
// What is the type of COO
// is there a better way to create it?
COO coo = make_pair(t->i, t->j);
// Here you are copy the content of `neighbours[coo]`
// into buddies. Do you need a copy or can we just
// use a reference to the original value.
Buddies buddies = neighbours[coo];
// Prefer a reference to a pointer.
Tile *buddy = &this->field[buddies[i]];
// Here you are making a copy of buddy.
// Do you really need another copy?
this->chains[buddy->chainId].push_back(*buddy);
// Again you are copy the value of tile into chains.
this->chains[t->chainId].push_back(*t);
// Another copy of tiles is being put here.
this->chains[t->chainId].push_back(*t);
// And you are copying neighbours into the
// recursive call here.
this->traverse(&this->field[buddies[i]], neighbours);
Use of this
Your use of this
is not idiomatic.
this->chains[t->chainId].push_back(*t);
Its much easier to write:
chains[t->chainId].push_back(*t);
The only reason to use this
is to disambiguate the use of shadowed variables. But shadowed variables is an error waiting to happen. So it is best to avoid shadowed variables entirely (even turn on your errors to make sure they don't happen)
-Wall -Wextra -Werror
default