Here are some suggestions that may help you improve your code.
Be careful with signed and unsigned
In a few places, the code compares an int
x
or 'y' with std::size_t
values width
and height
. For consistency, it would be better to declare x
and y
also as std::size_t
. Even better, see the next suggestion.
Eliminate unused code
The createSourceNode
function is unused and can be eliminated. Code that isn't used doesn't need to be written or maintained, which leads to better overall code quality.
Write portable code
As noted in the comments:
Are you using MSVC? This is their non-standard extension that allows non-lvalues to be passed as lvalue-references.
This is very easy to fix by changing the constructor. Instead of this:
ZeroTileOutsideException(std::string& error_message)
use this:
ZeroTileOutsideException(std::string error_message)
Now it complies with standards, and should still work just fine in MSVC. By avoiding non-portable extensions, you make your code easier to port and maintain for years to come.
Eliminate unused variables
The variable n
in the constructor for
SlidingTilePuzzleNode` is defined but never used. Since unused variables are a sign of poor code quality, you should seek to eliminate them. Your compiler is probably smart enough to warn you about such things if you know how to ask it to do so.
Use const
where practical
Generally, this code does a good job of using const
, but it should also be applied to the ostream<<
operator as in:
std::ostream& operator<<(std::ostream& out, const SlidingTilePuzzleNode& node)
This will require some changes to the code implementing it, but really should be possible to print the node without altering it. That also leads directly to the next suggestion.
Provide const
versions of access methods
In addtion to the two operator[]
functions you already have, I'd suggest adding these:
int SlidingTilePuzzleNodeRowSelector::operator[](std::size_t x) const
{
return m_node->m_state.at(m_offset_index + x);
}
const SlidingTilePuzzleNodeRowSelector& SlidingTilePuzzleNode::operator[](std::size_t y) const
{
return m_row_map.at(y);
}
This allows you to use the same handy notation in a const
-correct way.
Rethink the class interface
It's generall a bad idea to provide direct access to internal class data structures, and especially bad in a public interface. The non-const
versions of the functions mentioned above should be private
, for instance. Another useful thing to do would be to make SlidingTilePuzzleNodeRowSelector
a private class inside SlidingTilePuzzleNode
. See C.9 for more detail on that principle.
Don't use an exception
for unexceptional events
Users do all kinds of interesting things when interacting with computers, and in this case, it's not at all exceptional that a user would attempt an invalid move. For that reason, I would suggest instead that a return value from the various slide
methods indicating success or failure would make more sense.
Check the logic
I think there's is a problem with the logic of the moves. If we start with this:
1 2 3 4
5 6 7 8
9 10 11 12
13 14 15 0
-----------
And then mov "u" (for Up), the reult is this:
u
5 6 7 4
1 2 3 8
13 14 15 0
9 10 11 12
-----------
That's not typically how sliding puzzles actually work. If yours is intended to work this way, a bit more documentation for the user or as comments in the code at least, might be needed.