HiveBrain v1.2.0
Get Started
← Back to all entries
patterncppMinor

Texture managing

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
managingtexturestackoverflow

Problem

I'm new to C++ and SDL, and I've written a Texture manager class whose purpose is to help me manage sprites and other textures. I have a dispose method which unloads all the textures from a map container. It works correctly but I'd like to ask if I am not causing any memory leaks, or other pointer related issues.

TextureManager.h

class TextureManager
{
    public:

        bool load(std::string fileName, std::string id, SDL_Renderer* pRenderer);
        void draw(std::string id, int x, int y, int width, int height, int scale, SDL_Renderer* pRenderer,
                    SDL_RendererFlip flip = SDL_FLIP_NONE);
        void drawFrame(std::string id, int x, int y, int width, int height, int scale, int currentRow, int currentFrame, SDL_Renderer* pRenderer,
                    SDL_RendererFlip flip = SDL_FLIP_NONE);
        void dispose();

        std::map m_textureMap;

        static TextureManager* Instance()
        {
            if(s_pInstance == 0)
            {
                std::cout << "Created TextureManager singleton\n";

                s_pInstance = new TextureManager();
                return s_pInstance;
            }
            //point to same object (singleton pattern)
            return s_pInstance;
        }

        private:
            TextureManager(){}
            static TextureManager* s_pInstance;

            bool hasDisposed = false;
};

 typedef TextureManager TheTextureManager; //should be used when calling singleton

#endif // TEXTUREMANAGER_H


TextureManager.cpp

#include "TextureManager.h"

TextureManager* TextureManager::s_pInstance = 0;

bool TextureManager::load(std::string fileName, std::string id, SDL_Renderer* pRenderer){

    std::cout ::iterator TextureIterator;

    for(TextureIterator it = m_textureMap.begin(); it != m_textureMap.end(); ++it){
        std::cout first second second);//destroy texture
        std::cout  destroyed !\n";
    }

    hasDisposed = true;//cannot dispose more then once!
}

Solution

Over Complex Singelton

This is over complex because you are using pointers.

It also leaks resources at the end (luckily no destructor) but there is a principle holds. You should make sure you objects are correctly destroyed.

static TextureManager* Instance()
    {
        if(s_pInstance == 0)
        {
            std::cout << "Created TextureManager singleton\n";

            s_pInstance = new TextureManager();
            return s_pInstance;
        }
        //point to same object (singleton pattern)
        return s_pInstance;
    }
    static TextureManager* s_pInstance;


The classic Singelton pattern looks like this:

static TextureManager& instance()
    {
        // Notice this is a function static member.
        // This means it is created the first time instance() is called
        // and correctly destroyed at the end of the application.
        static TextureManager instance;
        return instance;
    }


Singeltons and copying

You don't want your singelton copied so disable the copy and assignment operators.

class TextureManager
{
    // In C++11
    TextureManager(TextureManager const&) = delete;
    TextureManager& operator=(TextureManager const&) = delete;

    // In C++03
    private:
    TextureManager(TextureManager const&); /* Don't define */
    TextureManager& operator=(TextureManager const&); /* Don't define */


Singelton is an anti-pattern

The singelton is a bit of an anti-pattern. They make testing hard.

It is often best to combine a singelton with a creation pattern (to allow yourself the luxury of creating different types of singelton for different things (ie testing)).

But even better is not to use a singelton. Pass the Texture manager around by reference.

Prefer not to pass pointers.

Pointers do not convey ownership information. So the user of your class does not know if they are passing ownership of the object to your methods (or even if the object should be dynamically allocated).

// This interface
 bool load(std::string fileName, std::string id, SDL_Renderer* pRenderer);


Is the pRenderer object allocated dynamically? Is your function taking ownership and will release the object when it is finished. Is your object going to take shared ownership of the object (if so when does it release its claim on ownership) how do I know its safe to delete this resource if you are keeping a copy?

Look up Smart pointers and ownership symantics.

Code Snippets

static TextureManager* Instance()
    {
        if(s_pInstance == 0)
        {
            std::cout << "Created TextureManager singleton\n";

            s_pInstance = new TextureManager();
            return s_pInstance;
        }
        //point to same object (singleton pattern)
        return s_pInstance;
    }
    static TextureManager* s_pInstance;
static TextureManager& instance()
    {
        // Notice this is a function static member.
        // This means it is created the first time instance() is called
        // and correctly destroyed at the end of the application.
        static TextureManager instance;
        return instance;
    }
class TextureManager
{
    // In C++11
    TextureManager(TextureManager const&) = delete;
    TextureManager& operator=(TextureManager const&) = delete;

    // In C++03
    private:
    TextureManager(TextureManager const&); /* Don't define */
    TextureManager& operator=(TextureManager const&); /* Don't define */
// This interface
 bool load(std::string fileName, std::string id, SDL_Renderer* pRenderer);

Context

StackExchange Code Review Q#87367, answer score: 8

Revisions (0)

No revisions yet.