Sprite cache and Directory reader

Question

I have been fiddling around with creating my own Sprite Cache. Currently, I am using the C header file, dirent.h, to open up a directory and load all .bmp files. I do not have to check if the file ends with .bmp, as SDL_LoadBMP will return a null pointer if it can not read the supplied file as a bitmap image.

One thing I do not like is having to look up sprites by their name. I feel like I could have it faster by not using strings, but then I do not know how I would be able to tell which image is which (AFAIK, dirent.h reads in the files in the directory in whatever order they appear in said directory. So treating the first file as ID=0 would not be wise, as ID=0 could be a different file if the user has a different file order in that directory.)

I have the Sprite Cache separated into two different objects - a directory reader, and the SpriteStore. The directory reader is responsible for receiving a directory and loading all the .bmp files into a map, while the SpriteStore is just responsible for handing out the requested sprites, or a nullptr if the sprite can not be found.

Directory Reader object

//DirectoryReader object is responsible for reading an entire directory
//And loading the sprites into a map<string, SDL_Surface*> object store. 
class DirectoryReader
{
public:
    DirectoryReader(std::string dir, std::map<std::string, SDL_Surface*>& directory_map)
    {
        ReadDirectory(dir, directory_map);
    }

private:
    void ReadDirectory(std::string dir, std::map<std::string, SDL_Surface*>& directory_map)
    {
        //Directory entry point and directory pointer
        struct dirent* directory_entry;
        DIR* directory_pointer = opendir(dir.c_str());

        //Vector of std::pair<int, string> representing each file in the directory
        //And the order number it appeared as in the directory
        std::vector<std::pair<std::string, SDL_Surface*> > pair_vector;

        //Loop thru the directory 
        while((directory_entry = readdir(directory_pointer)) != nullptr)
        {
            //Check to make sure the directory_entry is at a regular file
            if(directory_entry->d_type == DT_REG)
            {
                //Attempt to create an SDL_Surface object
                //SDL_LoadBMP returns null if it can not load a bitmap image
                SDL_Surface* surface = SDL_LoadBMP(directory_entry->d_name);
                if(surface != nullptr)
                    pair_vector.emplace_back(std::make_pair(std::string(directory_entry->d_name), surface));

            }

        }

        //Insert the pairs into the map
        for(auto p : pair_vector)
        {
            directory_map.insert(p);
        }

    }

    DirectoryReader() = delete;
    DirectoryReader(DirectoryReader& copy) = delete;
    DirectoryReader(DirectoryReader&& move) = delete;
    DirectoryReader& operator=(const DirectoryReader& copy) = delete;

};

Sprite Cache object

//SpriteStore is responsible for holding all loaded sprites in memory
//To be passed to components who need a sprite
class SpriteStore
{
public:
    SpriteStore(std::string directory) : directoryReader_uptr(nullptr)
    { 
        directoryReader_uptr.reset(new DirectoryReader(directory, store));
    }

    ~SpriteStore()
        {
        //Free every surface
        for(auto sprite : store)
            {
                SDL_FreeSurface(sprite.second);
            }

        //Clean the map
            map.clear();
        }

    //Attemp to return the sprite. If not found, return a nullptr.
    SDL_Surface* GetSprite(std::string ID)
    {
        try
        {
            return store.at(ID).get();
        }
        //The ID was not found
        catch(std::out_of_range ex)
        {
            return nullptr;
        }
    }

private:
    std::unique_ptr<DirectoryReader> directoryReader_uptr;
    std::map<std::string, SDL_Surface*> store;

    SpriteStore() = delete;
    SpriteStore(SpriteStore& copy) = delete;
    SpriteStore(SpriteStore&& move) = delete;
    SpriteStore& operator=(const SpriteStore& copy) = delete;

};

Answer 1

You don't always have to use a class

Unlike other OOP languages that have disallowed the declaration of free functions, C++ (luckily) didn't. Using classes makes sense when you need to keep some state around after calling a method. That is clearly not the case with DirectoryReader. It doesn't keep any intermediate state, which is good. ReadDirectory has a clean set of inputs and outputs (it is what is known as a pure function). Pure functions are preferable over objects, because they are easy to debug and reason about, since any call to such function with a given input must produce the same output (unless of course, the algorithms are broken, but then you'll know just from looking at the data!).

So if you extract ReadDirectory, making it a free function and ditching DirectoryReader, you will:

• Make code simpler and easier to understand even for those without knowledge about classes and objects.

• Still keep the code well structured and free of global state.

• Get rid of that member directoryReader_uptr that is being allocated dynamically, saving some memory and making SpriteStore simpler in the process.

Notice that I use the words "simple"/"simpler" a lot. Keeping it simple is one of the most important things to take into account when writing any kind of software.

Other things concerning ReadDirectory

You didn't check the return of opendir here:

DIR* directory_pointer = opendir(dir.c_str());

But if I'm not mistaken, it returns null for an invalid path. You should handle that case.

Why keep the vector pair_vector and then copy it to the output map at the end? You can just insert() in the map directly. That will reduce the size of the function in half, also making it simpler.

Your comments are too verbose. You don't have to detail every action, the code is well written and readable. One of our Code Review fellows wrote a very good blog post about this the other day, I suggest reading, it is short and makes a point: Writing Readable Code.

Since ReadDirectory only needs to read the path string you pass to it, take that parameter by const reference (const std::string &) and avoid a copy of the param into the function. Remember that C++ assumes the default to be a copy when you pass a class/struct type as a method/function parameter.

I can see that you are targeting C++11, so you no longer have to worry about expensive copies when returning an object from a function. This is specially true for the Standard containers, like map, which are all movable. You can refactor the function to return a map by value and the compiler will either move it out of the function or apply RVO. That gives your function a more natural calling syntax:

auto directoryEntries = ReadDirectory("path/to/dir/");

Looking into SpriteStore ...

As mentioned before, you should replace directoryReader_uptr by a free function. But it is worth noting that if it was to be kept, you shouldn't declare it as a pointer. If an instance of an object can be declared by value (as opposed, by pointer), like you did with the store map, then it is to be declared by value. Don't dynamically allocate when a plain value will do.

You are disabling more constructors/operators than it is needed to make the class non-copyable:

SpriteStore() = delete; // don't need
SpriteStore(SpriteStore& copy) = delete;  // not quite right
SpriteStore(SpriteStore&& move) = delete; // don't need
SpriteStore& operator=(const SpriteStore& copy) = delete;

If you delete the copy constructor and the assignment operator, the type will not be movable by default, so you don't have to explicitly delete the move ones.

Your declaration of the copy constructor is not correct. It must take a const reference. Your compiler might not be recognising that constructor as the default copy and it might be generating a default one, allowing the class to be copyable. Fix it to be like the assignment op: const SpriteStore& copy.

Deleting the default parameter-less constructor is also redundant. When you declare one that takes parameters, the implicit empty constructor is no longer provided.

GetSprite, etc

When you want to test if a value is in the map don't use at and let an exception get thrown to catch is right away. Instead, find the key to check if the value is there:

SDL_Surface* GetSprite(const std::string & ID) // Note the const ref here to prevent an unnecessary copy
{
    auto iter = store.find(ID); // Returns map.end() iterator if not found
    if (iter == std::end(store))
    {
        return nullptr;
    }
    return iter->second;
}

Lastly, no need map.clear() in the destructor. Just free the SDL pointers and you are done. The map clears itself, so you are only duplicating work there.