Simple 8 Puzzle - ASCII - follow up

Based on the previous question, I have implemented all suggestions with slight modifications. The game seems fine, are there any further improvements needed in this game?

```
#include
#include
#include
#include
#include
#include
#include
#include

template
struct Vector
{
Vector(T x, T y): x(x), y(y){}
T x, y;
};

using Vector2i = Vector;
using Vector2u = Vector;

namespace Key
{
enum Type
{
Up = 72,
Right = 77,
Left = 75,
Down = 80
};
}

namespace Direction
{
enum Type
{
Up,
Down,
Left,
Right,
DirectionCount
};
}

namespace utility
{
std::mt19937 randomEngine()
{
std::array seed_data;
thread_local std::random_device source;
std::generate(seed_data.begin(), seed_data.end(), std::ref(source));
std::seed_seq seeds(seed_data.begin(), seed_data.end());
thread_local std::mt19937 seeded_engine(seeds);
return seeded_engine;
}

Vector2i directionOffset(Direction::Type direction)
{
using OffsetHolder = std::array;

constexpr static OffsetHolder xOffset{ 0, 0, +1, -1 };
constexpr static OffsetHolder yOffset{ +1, -1, 0, 0 };

return{ xOffset[direction], yOffset[direction] };
}
}

namespace ut = utility;

class Puzzle
{
private:
constexpr static auto MaxTiles = 9u;
constexpr static auto Columns = 3u;

using Pair = std::pair;
using TileHolder = std::array;

public:
Puzzle()
{
std::iota(mTiles.begin(), mTiles.end(), '1');
mTiles.back() = ' ';

thread_local auto random = ut::randomEngine();

std::shuffle(mTiles.begin(), mTiles.end(), random);
}

void update(Direction::Type direction)
{
auto coord = getEmptyTileCoordinates();

auto x = coord.first.x + ut::directionOffset(direction).x;
auto y = coord.first.y + ut::directionOffset(direction).y;

if (x >= 3u || y >= 3u) re

I see a number of things that may help you improve your code.

Don't use system("cls")

There are two reasons not to use system("cls") or system("pause"). The first is that it is not portable to other operating systems which you may or may not care about now. The second is that it's a security hole, which you absolutely must care about. Specifically, if some program is defined and named cls or pause, your program will execute that program instead of what you intend, and that other program could be anything. First, isolate these into a seperate functions cls() and pause() and then modify your code to call those functions instead of system. Then rewrite the contents of those functions to do what you want using C++. For example, if your terminal supports ANSI Escape sequences, you could use this:

void cls()
{
    std::cout << "\x1b[2J";
}

Isolate platform-specific code

In this code, there are several things that are DOS/Windows only including #include and the getch() and kbhit() functions within that, and also system("cls"); that I've already mentioned. Your code runs successfully on Linux if I supply those missing functions, but it would be nice if there were an #ifdef WINDOWS already in the code so that one could recompile without having to alter the source code.

Make sure you have all required #includes

The code uses std::ref but doesn't #include .

Fix the initialization

The initialization of the puzzle does not always result in a solvable puzzle. In fact, exactly half of all possible states are impossible to solve. This is a bit frustrating for anyone actually trying to solve the puzzle, so I'd recommend a different initialization that only results in solvable puzzles.

Don't abuse namespaces

The namespacess in this program seem excessive to me. In particular, I was annoyed by this line:

namespace ut = utility;

If you find the name utility too long, change it. If you don't, then use it as is. It's only used three places anyway.

Don't abuse auto

I'm a fan of auto generally, but when I see lines like this:

auto column = 0u;

it makes me question why it's being used. It seems to be written almost as though the point were obfuscation.

Write code for clarity

Let's imagine that you've just been asked to add a feature to this code and it's the first time you've seen it. You look at this line in Puzzle:

Pair getEmptyTileCoordinates() const

So what is a Pair? You scan the code and find this line:

using Pair = std::pair;

Great. So what's a Vector2u? More hunting until you find this:

using Vector2u = Vector;

By now you are cursing the name of the original programmer. What is a Vector? It's this:

template
struct Vector
{
    Vector(T x, T y): x(x), y(y){}
    T x, y;
};

So now you're four levels of indirection away and have probably forgotten what started you on this chain of lookups in the first place. Even more frustrating is that each of these is only used exactly once in the program, so there's little point to most of it. Don't do this! Write for clarity instead.

Keep private data together

Both the Game and the Puzzle classes have three different places where private: is used. Again, this makes things more confusing than they need to be. Instead, put all of the private parts together. I recommend putting functions first and then data.

Make static data static

The mKeyBinding member of the doesn't change (so it should be const) and is also invariant across Puzzle instantiations. This strongly suggests that it should instead be static. Change the declaration to this:

static const std::map mKeyBinding;

Then outside the class, initialize it like this:

const std::map Game::mKeyBinding{ 
    {Key::Up, Direction::Up},
    {Key::Down, Direction::Down},
    {Key::Left, Direction::Left},
    {Key::Right, Direction::Right}
};

Note that I've also dropped yet another typedef to clarify the code.

Place friend declarations in a public section

A friend declaration is always public no matter if it's declared within the private or public sections. For clarity, put it in the public section unless there is a very good reason not to (and there is almost never a good reason).

StackExchange Code Review Q#117766, answer score: 4