10
\$\begingroup\$

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 <iostream>
#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 = '.';
    static const char WallChar = '=';
    static const char StartChar = 'x';
};

Level.cpp

#include "Level.h"
#include <fstream>
#include <sstream>

std::string Level::GetName(){
    // Should this return a Pointer?
    return _name;
}

int Level::GetPlayerStartX() {
    return _playerStartX;
}

int Level::GetPlayerStartY() {
    return _playerStartY;
}

Tile* Level::GetTiles() {
    return _tiles;
}

int GetArrayIndex(int line, int column){
    return (line * Level::MaxCols) + column;
}

Level* Level::LoadFromFile(std::string fileName){
    std::string line;
    std::ifstream file (fileName.c_str());

    // This news up a level and works with it as a pointer
    // Do callers need to call delete on it? How would they know?
    Level* level = new Level();

    int numBoxes = 0;
    int numTargets = 0;
    bool playerStart = false;

    // The Level File is a text file in which the first line
    // is the name and the remaining lines is a grid of tiles
    int y = -1;
    while (getline(file, line)) {
        if(y == -1){
            level->_name = line;
        }
        else if (y >= Level::MaxRows) {
            // Is there a better way? Like C#'s string.Format()
            std::ostringstream exceptionMessage;
            exceptionMessage << "There are more than " << Level::MaxRows << " rows in the level.";

            // I'm calling .str().c_str() - is that a possible leak?
            throw std::exception(exceptionMessage.str().c_str());
        }   
        else {
            int lineLength = line.length();

            if(lineLength> Level::MaxCols){
                 std::ostringstream exceptionMessage;
                 exceptionMessage << "There are more than " << Level::MaxCols << " tiles in a column.";

                throw std::exception(exceptionMessage.str().c_str());
            }

            for (int x=0; x < Level::MaxCols; x++) 
            {
                int ix = GetArrayIndex(y, x);

                // We always want "Full" lines (MaxCols length),
                // but the line in the file could be shorter    
                char c = ' ';
                if(x < lineLength) {
                    c = line[x];
                }

                switch(c) {
                    case Level::FreeChar:
                        level->_tiles[ix] = TILE_FREE;
                        break;
                    case Level::BoxChar:
                        numBoxes++;
                        level->_tiles[ix] = TILE_BOX;
                        break;
                    case Level::BoxOnTargetChar:
                        numBoxes++;
                        numTargets++;
                        level->_tiles[ix] = TILE_BOXONTARGET;
                        break;
                    case Level::TargetChar:
                        numTargets++;
                        level->_tiles[ix] = TILE_TARGET;
                        break;
                    case Level::WallChar:
                        level->_tiles[ix] = TILE_WALL;
                        break;
                    case Level::StartChar:
                        if(playerStart) {
                            throw std::exception("There is more than 1 player start in the level.");
                        }
                        playerStart = true;
                        level->_playerStartX = x;
                        level->_playerStartY = y;
                        level->_tiles[ix] = TILE_FREE;
                        break;
                    default:
                        throw std::exception("Invalid Character in Level: " + c);
                }
            }
        }
        y++;
    }

    // Fill up any missing Rows - the array is always filled to MaxRows * MaxCols
    for(; y < Level::MaxRows; y++){
        for(int x = 0; x < Level::MaxCols; x++){
            int ix = GetArrayIndex(y, x);
            level->_tiles[ix] = TILE_FREE;
        }
    }

    if(numBoxes != numTargets) 
    {
        std::ostringstream exceptionMessage;
        exceptionMessage << "There are " << numBoxes << ", but " << numTargets << " in the level.";

        throw std::exception(exceptionMessage.str().c_str());
    }

    return level;
}

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

Level::~Level(){
    if(_tiles) {
        // Since _tiles is an array, I need delete[] instead of delete?
        delete[] _tiles;
    }
}
\$\endgroup\$
  • \$\begingroup\$ Do you plan for LoadFromFile to do caching, or would calling it twice with same file name just load it again? If you're not caching, I would just return it by value and not worry about memory management at all. \$\endgroup\$ – Corbin Sep 8 '14 at 23:36
  • \$\begingroup\$ @Corbin no caching, every call re-reads the file. \$\endgroup\$ – Michael Stum Sep 9 '14 at 1:49
13
\$\begingroup\$

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<Tile> 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<Level> 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<Level> 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.

\$\endgroup\$
11
\$\begingroup\$

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

To start with it should never return a pointer.
Pointers do not convey ownership semantics and thus it is not clear who the owner is and thus who is responsible for deleting the object.

If you can only have one level per user then I would return a std::unique_ptr<Level> this will indicate that ownership is being returned to the caller and thus he is the owner and thus responsable for deleting (of course the unique_ptr will also do all the work).

Alternatively. Lets say you had a multiplayer game where multiple users shared the level object. Then you have two options.

  • You can retain ownership of the Level objects and return references to the players.
  • You can return a std::shared_ptr to indicate that ownership is shared (again the shared_ptr will do all the work).

Is the caller of Level::LoadFromFile responsible for calling delete level?

Your interface does not make that clear. Which is why you should not use pointers. I think I covered all this in the previous paragraph.

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

True and slightly outdated. Every call to new requires a call to delete. BUT usually the delete is automated by using a smart pointer, or alternatively you don't use new/delete directly but store objects in a container (and the container handles the memory management aspects for you.

Modern C++ has very few direct delete calls (as this is not exception safe).

but how would the caller know about that requirement without reading the documentation?

That's why we use smart pointers and containers. So the user does not need to read the documentation.

Am I handling the internal _tiles array correctly?

No. Because you have a new in a constructor and delete in the destructor. You MUST also define the copy and assignment operators as well. Look up the Rule of Three.

But I would skip all that and use a std::vector<Tile> and let the vector do the memory management for you.

There is a concept called separation of roles. This basically states that a class should either do resource management or business(game) logic. A single class should never do both. Unfortunately you are doing both in Level. By using std::vector to handle your resource management (memory management of the Tiles) you can make sure that Level only needs to handle business logic.

The constructor of level new's it, so it's the responsibility of the Level class to delete[] it in the destructor?

Yes. But don't make the delete conditional. It is OK to call delete on a NULL pointer.

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

Couple of hidden memory problems.

Level*  l1  = Level::LoadFromFile("Plop");
Level   copy1(*l1);    // You did not expect that would work.
Level   copy2(copy1);  // Just disable them if you don't want copying to work.

Anyway. You now have three objects that all container a pointer to Tile (the same pointer). When each goes out of scope there destructor will call delete on the same pointer.

static Level& Level::LoadFromFile(std::string fileName)
{
     // As an example of retaining ownership in the LoadFromFile()
     // If I had a multi-player game were players shared level
     // objects. I would use something like this.


     // Static function member.
     // Could arguably by a static member of the class.
     static std::map<std::string, Level>  loadedLevels;

     // See if it already exists.
     auto find = loadedLevels.find(fileName);
     if (find != loadedLevels.end())
     {
         return find->second;
     }

     // Construct a Level object in the map
     auto res = loadedLevels.emplace(fileName, fileName);
     return *res.first.second;
}
private:
Level(std::string const& fileName)
{
      // Move the work of construction into here.
}
\$\endgroup\$
10
\$\begingroup\$

Conveniently, all of the concerns in your questions can be solved by replacing all new and delete operations with a storage container that manages memory automatically. In C++, it's best to avoid handling the memory yourself since memory leaks can occur if you do not do it properly (and it is pretty easy to get it wrong, especially in larger programs with a lot of memory work). You'll also have to deal with handling ownership, which is why raw points are also not recommended in C++ if you have access to smart pointers available in C++11.

Since you're dealing with a dynamically-allocated array, one common option is to use an std::vector. It's essentially a wrapper around a dynamically-allocated array that handles all the memory for you.

Instead of this:

Tile* _tiles;

you would have this:

std::vector<Tile> _tiles;

The syntax around this is still the same (such as random access), but you now have member functions to help utilize these objects idiomatically. Again, you won't be dealing with new and delete since they'll clean up after themselves. Of course, be sure to read the documentation to learn more about it.

After that, you won't need your destructor anymore. If you don't need one, you're not required to provide one as the compiler will provide a default one.

\$\endgroup\$
  • \$\begingroup\$ Thanks! Would I just pass the std::vector around if I want to expose it through GetTiles, or a pointer to it? (Asking because to my understanding, pass by value is the default and I want to make sure I'm not making copies of the vector every time someone calls GetTiles()) \$\endgroup\$ – Michael Stum Sep 8 '14 at 6:14
  • 1
    \$\begingroup\$ @MichaelStum: To prevent copies, you would pass by const&: func(Type const& object). Moreover, you should try to avoid "getters" since they're bad for encapsulation (they can expose implementation details). I could also address that, or I may let someone else do that. But for now, make these changes so that potential memory leaks can be eliminated. \$\endgroup\$ – Jamal Sep 8 '14 at 6:16
  • \$\begingroup\$ In the final implementation I'll have Tile GetTileAt(int x, int y) and void SetTileAt(int x, int y, Tile t) instead of exposing the array. I'll read up on const&. \$\endgroup\$ – Michael Stum Sep 8 '14 at 6:22
8
\$\begingroup\$

I feel like I'm beating a dead horse at this point, but two more quick things to add about memory management:

  1. If you don't actually need to use dynamic allocations, don't. I don't currently see a reason why Level::LoadFromFile doesn't just return a Level by value.

  2. new, delete and exceptions do not play well together at all. Just consider this: if your parsing code throws an exception, what happens to the Level that was allocated by new a few lines earlier? You no longer have a pointer to it thus it's leaked. It's a weird concept coming into C++ since you think about C++ and immediately think "oh, raw memory!" but that's the exact opposite of ideal. Raw memory management should be incredibly rare, especially in non-library code.


Taking a filename when what you really want is a file is a second level dependency. Instead, you should take an std::istream (by reference: std::istream&), and use it. Then, if you wanted, you could build a convenience option on top of it that took a filepath.

In a general sense, creating your own dependencies implies that there is coupling going on and it absolutely murders the ability to easily test things. In a more real sense though, consider what would happen if you wanted to read from std::cin. You couldn't do it, even though that's essentially the same thing as reading from a file. Or what if you were unit testing and wanted to read from an std::istringstream? Once again, you can't. Taking a file, not a filepath, alleviates this problem.


_tiles = new Tile[TileCount]; is TileCount a constant? If so, you shouldn't be doing dynamic memory allocation. If it's a constant, either use an array, or use std::array. If it's not a constant, use std::vector.


// Is there a better way? Like C#'s string.Format()
std::ostringstream exceptionMessage;
exceptionMessage << "There are more than " << Level::MaxRows << " rows in the level.";

// I'm calling .str().c_str() - is that a possible leak?
throw std::exception(exceptionMessage.str().c_str());

Always try to throw the most specific type of exception you can. In this situation, I would either create your own as glampert suggested, or I would throw a std::runtime_error. std::exception is at the very top of the exception hierarchy which means you can't catch it without catching every other type of exception too (well, those that also derive from std::exception). That catch-all behavior is typically problematic if more than one type of exception can be thrown.

As for your leak question... it's not actually a leak but the opposite situation, a dangling pointer.

Breaking it into smaller pieces makes it a bit clearer:

std::string str = exceptionMessage.str();
throw std::exception(str.c_str());

Exceptions unwind the stack, and the important part of that in this context is that they cleanup any automatically managed objects, in this case our string.

str.c_str() gives you a pointer to a buffer that std::string contains. Once that str stops existing, str.c_str() is no longer a valid pointer. Maybe a bit clearer with an example:

try {
    std::string str = ...;
    throw std::exception(str.c_str());
} catch (std::exception& ex) {
    std::cerr << ex.what() << std::endl; // This references the internals 
                                         // of a string that no longer exists!
}

(On a second reading, I realized I should point out that I've assumed that the string is not copied. If the string is copied into the exception then you're fine and there's no dangling pointer. Since exceptions often try to avoid operations that can throw [since then you enter the twilight zone and things get weird], I would assume it doesn't copy. There's no guarantee that it copies anyway, so you're basically forced to assume it doesn't unless you want fragile code.)


As a last note, your parsing is very complicated. Since the name reading is only done once, and it's always the first line, do it outside of the loop. Then, break your row reading and column reading apart. I would structure the top level something like this:

if (!std::getline(file, level->name_)) {
    throw std::runtime_error("could not read level name");
}
for (int lineIdx = 0; lineIdx < Level::MaxRows && std::getline(file, line); ++lineIdx) {
    parse_column(level, line);
}
if (std::getline(file, line)) {
    throw std::runtime_error("number of rows exceeded maximum");
}
\$\endgroup\$
  • \$\begingroup\$ Actually, I've just realized that there is something wrong here. std::exception is not supposed to take a const char*! \$\endgroup\$ – glampert Sep 9 '14 at 0:24
  • \$\begingroup\$ @glampert I'm actually fairly certain that it's an interface, not a class, but I wasn't sure enough to comment on it :/ \$\endgroup\$ – Corbin Sep 9 '14 at 0:36
  • 1
    \$\begingroup\$ Aha! here it is: "The constructors exception(const char * const &message) and exception(const char * const &message, int) are Microsoft extensions to the Standard C++ Library." \$\endgroup\$ – glampert Sep 9 '14 at 0:39
  • 1
    \$\begingroup\$ @glampert That certainly explains it. Gotta hate good old MS extensions x.x \$\endgroup\$ – Corbin Sep 9 '14 at 0:47
7
\$\begingroup\$

There are a few other questions that you have through the code as comments that weren't addressed by the previous replies, so I'll attempt to answer a few more. Note: I'll be assuming you have a C++ 11 capable compiler.

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

Yes, you could declare those constants directly inside the function if that is the only place they are required. This is good since it shortens the class interface (the header file).


std::string Level::GetName(){
    // Should this return a Pointer?
    return _name;
}

Returning by value is fine if you don't think there is going to be any overhead in a copy. Furthermore, C++ now has move semantics which can mitigate or even eliminate copy overhead.

That said, sometimes it might by necessary/useful to return by reference. Returning by reference would be valid in this case, since _name is a class member. Local variables cannot be returned by reference because they will cease to exist once the function returns.

If you wish to allow external modification of the class data, you can return by ref:

std::string & GetName() { return (_name); }
// caller can alter _name

If the intention is to provide read-only access, then return by const ref:

const std::string & GetName() const { return (_name); }
// caller can only read from _name

std::ifstream constructor now takes C++ strings so you don't need to call c_str() on fileName:

std::ifstream file(fileName);

In the implementation of LoadFromFile(), the idiom you have used of first allocating the Level object and then filling it up by accessing its properties (e.g.: level->_name = line;) is not ideal. A better approach would be to parse the file and store the read contents into local variables, then, at the end construct a Level passing parameters to the constructor. E.g.:

return new Level(tiles, name);

If you happen to throw an exception, you didn't construct a Level object for nothing.

For that, however, you should make the vector parameter a move reference, to avoid a copy. So the Level constructor would look like:

Level(std::vector<Tile> && tiles, std::string && mapName);

And you would call it like so:

return new Level(std::move(tiles), std::move(name));

I think it is a good thing that you get acquainted with the new move semantics from the start, since this is a very useful and powerful new feature of the language.


// Is there a better way? Like C#'s string.Format()
std::ostringstream exceptionMessage;
exceptionMessage << "There are more than " << Level::MaxRows << " rows in the level.";

To be honest with you, yes you can use a "format" syntax in C++. This can be done with functions like sprintf. However, this format system is nothing like the one in C#. It is in fact an inherited feature from C, therefore, is has several drawbacks as safety issues. My advice, don't use them. You don't have to.

In this particular case, you could use a shorter solution with a plain std::string. Use one of the conversion functions like std::to_string():

const std::string exceptionMessage = std::string("There are more than ") + std::to_string(Level::MaxRows) + std::string(" rows in the level.");

You do have to add the std::string("...") casts however. Otherwise the compiler would interpret the string literals as pointers to char arrays (const char *). And you cannot concatenate char arrays with the operator +.


// I'm calling .str().c_str() - is that a possible leak?
throw std::exception(exceptionMessage.str().c_str());

I'm not sure. Your compiler seems to be using a non-conforming implementation of std::exception! (Visual Studio is one case). If you look at the documentation here and here, you will see that it is not supposed to have a constructor that accepts a const char *, nor an std::string. std::exception is a base class for more specific exception types; it should be just an interface class.

It is very likely that the implementation that you happen to be using will copy the input string, but this is not standard, so there are no guarantees.


Consider defining a custom exception type. You might want to define a custom exception type in the future to make error reporting more clear/specific. Just make sure you inherit the specialized exception type from std::exception.


Finally, the naming convention you've used for the member vars. This is a very thorough discussion on the subject. The fact is that names starting with an underscore _ in the global scope are reserved for use by the C++ implementation. In the class scope, this should be fine, however, I discourage the use of such naming convention. I would suggest camelCase for member variables as well. If you really like to have some distinction between member data and locals, the m_ prefix is quite popular. Also appending an underscore at the end seems to be well accepted: memberVariable_.

\$\endgroup\$
  • 1
    \$\begingroup\$ I've looked into the implementation of Visual Studio's std::exception ctor, and indeed it copies the incoming string. \$\endgroup\$ – Michael Stum Sep 10 '14 at 5:31
  • 1
    \$\begingroup\$ @MichaelStum, yes that was to be expected, otherwise it would render the ctor pretty much useless. So you don't have to worry about leaks or stale pointers. The only real issue is portability. It probably won't compile outside of VS. If you're not writing multi-platform code, this is not a big deal. \$\endgroup\$ – glampert Sep 10 '14 at 13:41

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.