Skip to main content
Code-quote quoted code
Source Link
Toby Speight
  • 81.7k
  • 14
  • 101
  • 308

This is very easy to fix by changing the constructor. Instead of this: ZeroTileOutsideException(std::string& error_message)

ZeroTileOutsideException(std::string& error_message)

The variable n in the constructor for the constructor for SlidingTilePuzzleNodeSlidingTilePuzzleNode` 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.

This is very easy to fix by changing the constructor. Instead of this: ZeroTileOutsideException(std::string& error_message)

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.

This is very easy to fix by changing the constructor. Instead of this:

ZeroTileOutsideException(std::string& error_message)

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.

Source Link
Edward
  • 66.6k
  • 4
  • 118
  • 282

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.