Is this a modern C++11 implementation of a Level class and static factory?

This is a follow up to my previous question, now that I've read up more about modern C++ (specifically C++11) and the answers in the question. I'm using MSVC/Visual Studio 2012 Update 4.

Specific things I'm wondering about:

• Am I doing the Exception right? I asked on SO regarding an access problem I had, but I also want to know if this is proper style.

• Could there be any portability problems with the enum class Tile, specifically with the fact that the values are chars that correspond to the text file on disk

• Should the ints that GetTileAt and SetTileAt take be declared const? Since they are copied by value anyway, that seems superflous?

• Have I done the Level ctor right? Based on glampert's suggestion I'm using std::move to only construct it at the very end of the LoadLevelFromFile method

Level.h

#ifndef WAREHOUSE_LEVEL_H
#define WAREHOUSE_LEVEL_H

#include
#include
#include
#include

enum class Tile : char {
Unknown = 0,
Free = ' ',
Box = '+',
Target = '.',
Wall = '=',
BoxOnTarget = '*',
PlayerStart = 'x'
};

class LevelLoadException : public std::exception
{
public:
LevelLoadException(std::string msg) : m_message(msg) { }
const char* what () const throw ()
{
return m_message.c_str();
}
private:
std::string m_message;
};

class Level
{
public:
const std::string& GetName() const;
int GetPlayerStartX() const;
int GetPlayerStartY() const;
Tile GetTileAt(const int x, const int y) const;
void SetTileAt(const int x, const int y, Tile tile);
static std::unique_ptr LoadFromFile(const std::string& fileName);

static const int MaxCols = 30;
static const int MaxRows = 20;
static const int TileCount = MaxCols * MaxRows;

Level(std::vector&& tiles, std::string&& mapName, int playerStartX, int playerStartY);
private:
std::vector m_tiles;
int m_playerStartX, m_playerStartY;
std::string m_name;
};

#endif


Level.cpp

`#include "Level.h"
#

General remarks:

-
Since you’re using C++11, use uniform initialisation syntax to initialise variables and class members.

-
The throw() specification is deprecated and replaced by noexcept.

-
The bracing style is somewhat inconsistent (new line or same line?).

-
Inheriting from std::exception obviates need for what, m_message:

class LevelLoadException : public std::exception {
public:
    LevelLoadException(std::string msg) : std::exception{msg} {}
};

(Note the use of uniform initialisation syntax.)

-
Getters and especially setters are generally considered harmful in C++. This is not a rigid rule, but in your case there are better options:

-
Refactor {Get,Set}TileAt into an operator()(int, int), and maybe put it into an own class rather than Level (single responsibility …). Also consider whether the setter is needed at all. In any case, usage should look like this:

Level lvl;
// …
Tile a = lvl(1, 2);
lvl(1, 2) = Tile::Free;

or

Tile b = lvl.tiles(1, 2);
lvl.tiles(1, 2) = Tile::Free;

-
GetPlayerStart{X,Y} should be one function and return a Point (or, at the minimum, a std::tuple). And there’s no need for the Get prefix; the same goes for GetName. PlayerStart and Name are sufficient and idiomatic (compare std::vector::length, std::stringstream::str etc.).

-
LoadFromFile should return its result by value, not pointer.

-
Consider using constexpr for the static const ints.

-
Consider only offering a single means of construction:

• Either via overloaded constructors

• or via static creator functions (LoadFromFile, Load)

Constructor would be more canonical C++ but in this case static functions are arguably more readable. Either way, mixing construction via functions and constructors makes the interface inconsistent.

-
I would probably rewrite LoadFromFile slightly, incorporating several changes. Since the code is long-ish, I’ve uploaded it as a gist. the salient changes are:

• Use range-based for loop, treat x and y iterating analogously to each other.

• Use prefix increment instead of postfix. It doesn’t make a difference for int but it can make a difference in general, and using prefix increment is never less efficient (and sometimes more).

• Separate parsing of level name and tiles

• Remove code duplication (assigning tile)

Answering your specific question:

-
Modulo what I’ve said above, the exception(-usage) looks good.

-
The enum class is fine, there’s no portability problem. I’m somewhat surprised that you can use the constants as switch labels for a char (there’s normally no implicit conversion between an enum class and its underlying type).

-
For arguments passed by value, adding const to the argument type in the prototype does nothing and serves no purpose. For the implementation, there’s a difference (and I argue that all arguments should be passed as const, but I’m inconsistent in enforcing this in my own code): a const argument cannot be modified inside the function, which is usually desired. But at any rate, a modification would only be visible inside the class because the argument is a copy of the value passed in by the user.

-
The constructor looks OK-ish but rvalue references prevent calling the constructor with variable:

std::vector tiles{…};
std::string mapname{"Test"};
Level lvl{tiles, mapname, 0, 0};

This doesn’t compile – you’d need to move the values and this is cumbersome and might not be desired anyway.

There’s a simple solution: just accept the argument by value. That way, they’ll be copied when necessary (in the above case), and moved where possible (rvalue reference, such as from a temporary object). However, you now have to move them explicitly into their members:

Level::Level(std::vector tiles, std::string mapName, int playerStartX, int playerStartY)
    : m_tiles{std::move(tiles)}
    , m_name{std::move(mapName)}
    , m_playerStartX{playerStartX}
    , m_playerStartY{playerStartY} {}

(And again, consider using a Point object for the player start position.)

StackExchange Code Review Q#62493, answer score: 6