Static factory function and lifetime

I'm trying to teach myself C++ at the moment, after years of C# and other managed languages. The class in question is a Level in a game, and the idea is to instantiate it from a static factory function.

The questions that I have is:

• Should Level::LoadFromFile return a Level or a pointer to one, as it does now?

• Is the caller of Level::LoadFromFile responsible for calling delete level? After all, the factory function uses new, and from what I learned in C++, new always requires delete except with arrays where delete[] is required - but how would the caller know about that requirement without reading the documentation?

• Am I handling the internal _tiles array correctly? The constructor of level new's it, so it's the responsibility of the Level class to delete[] it in the destructor?

• Am I doing anything that could introduce a memory leak or a hidden bug?

Tile.h

// Do I need to prevent multiple inclusion? In what situations?
#ifndef TILE
#define TILE

enum Tile { TILE_UNKNOWN, TILE_FREE, TILE_BOX, TILE_TARGET, TILE_WALL, TILE_BOXONTARGET };

#endif

Level.h

```
#include
#include "Tile.h"

class Level
{
public:
std::string GetName();
int GetPlayerStartX();
int GetPlayerStartY();
Tile* GetTiles();

// Static Factory Function - only way to instantiate
static Level* LoadFromFile(std::string fileName);

// Static Consts for external callers to know constraints
static const int MaxCols = 30;
static const int MaxRows = 20;
static const int TileCount = MaxCols * MaxRows;

~Level();

private:
// Private ctor to prevent instantiation
Level();

// An Array of TileCount tiles
Tile* _tiles;

int _playerStartX, _playerStartY;
std::string _name;

// Used by LoadFromFile - could possibly moved into that function?
static const char FreeChar = ' ';
static const char BoxChar = '+';
static const char BoxOnTargetChar = '*';
static const char TargetChar = '

Tile.h

Answering the question in the comment, yes you always need an include
guard for a file if you plan on including it in more than one .cpp.
Note: #pragma once is also viable.

BTW, Tile.h is so small, maybe remove it an place the Tile enum
inside Level.h.

C++ 11 enums:

C++ 11 introduced typed enums, very similar to the ones in C#, so you will
certainly prefer them (Hopefully your compiler is C++ 11 enabled). You can redeclare Tile using enum class:

enum class Tile 
{
    Unknown, 
    Free, 
    Box, 
    Target, 
    Wall, 
    BoxOnTarget
};

An now you use them as such:

Tile t = Tile::BoxOnTarget;

Const correctness:

In C++, class methods that do not modify any member state, but still read
some member state should be made const. Read the link above for more on const correctness.

Get*() methods are usually the candidates for this:

class Level
{
public:
    std::string GetName() const;
    int GetPlayerStartX() const;
    int GetPlayerStartY() const;
    Tile* GetTiles() const;

    // Also in the .cpp definitions
    ...

This has the benefit of making your code less error-prone. If you
were to accidentally attempt to modify any class state inside those
getters, you would get a compiler error.

Memory management:

Memory management is a big deal in C++, but also, it is pretty hard to
get right. This is why we must automate as much as possible.

Always use the standard containers for collections. Replace Tiles * _tiles
with std::vector tiles.

When allocating individual objects, don't use raw pointers, use
a smart pointer.

static Level* LoadFromFile(std::string fileName);

Should return a smart pointer to Level. Namely, a unique_ptr.
This makes ownership semantics very clear and ensures automatic
cleanup (BTW, this is a form of garbage collection).

static std::unique_ptr LoadFromFile(std::string fileName);

Note: unique_ptr was introduced by C++ 11, prior to that, there was auto_ptr. auto_ptr is still probably better than a raw pointer, but only use it if you don't have access to unique_ptr.

Pass by reference whenever it makes sense:

When we are not copying an object from a function parameter,
we pass them by reference.

static Level* LoadFromFile(std::string fileName);

fileName is currently being copyed. Which is the default in C++.
This is slightly wasteful, since it is a read-only variable.
Copying is perfectly fine for small objects, like ints and tiny
structs, but a string can potentially be quite large.
So pass by const reference when only reading from an object (the & indicates a reference):

static unique_ptr LoadFromFile(const std::string & fileName);

Minor details:

These constants would look better if you aligned the = signs:

static const char FreeChar        = ' ';
static const char BoxChar         = '+';
static const char BoxOnTargetChar = '*';
static const char TargetChar      = '.';
static const char WallChar        = '=';
static const char StartChar       = 'x';

You could also make them an enum if you prefer:

enum class Char 
{
    Free        = ' ',
    Box         = '+',
    BoxOnTarget = '*',
    Target      = '.',
    Wall        = '=',
    Start       = 'x'
};

In the constructor, the proper way of initialing member data is:

Level::Level() 
    : _playerStartX(0)
    , _playerStartY(0)
    , _tiles(new Tile[TileCount])
{
}

That list-like syntax is a C++ particularity that Java and C# don't share, as far as I know.

StackExchange Code Review Q#62254, answer score: 13