Using std::vectors to store textures I need to write a TextureManager class which stores Texture objects; the problem is I'm using an std::map&lt;Texture *, const char*&gt; map to store Texture objects using a key.

Searching elements through a map is not ideal because of performance issues (I'm writing a C++ library, so performance is important).

I've had the idea to use an std::vector&lt;Texture*&gt;. In this case, I need to use an unsigned int. But after several tries, I still cannot implement it.

Here is the .h:

class CTextureManager
{
public:
    CTextureManager(SDL_Renderer *pRenderer);
    void SetRenderer(SDL_Renderer *pRenderer);

    ~CTextureManager();

    // Load File function
    CTexture *LoadFromFile(std::string name, std::string filename);

    bool UnLoad(std::string name);

    CTexture *GetTexture(std::string name);

    void ClearAll();


protected:
    std::map&lt;std::string, CTexture *&gt; m_mapTexture;
    SDL_Renderer *m_pRenderer;

};


and the .cpp:

CTextureManager::CTextureManager(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

CTextureManager::~CTextureManager()
{
    ClearAll();

    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }
}

void CTextureManager::ClearAll()
{
    for (std::map&lt;std::string, CTexture *&gt;::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)
    {
        delete (it-&gt;second);
        (it-&gt;second) = nullptr;
    }
    m_mapTexture.clear();
}

CTexture * CTextureManager::GetTexture(std::string name)
{
    return m_mapTexture[name];
}

bool CTextureManager::UnLoad(std::string name)
{
    CTexture *pTempTexture = m_mapTexture[name];

    if (!pTempTexture)
        return false;

    delete pTempTexture;
    pTempTexture = nullptr;
    m_mapTexture.erase(name);
    return true;
}


CTexture * CTextureManager::LoadFromFile(std::string name, std::string filename)
{
    CTexture *pTexture = m_mapTexture[name];

    if (pTexture == nullptr)
    {
        pTexture = new CTexture();
        pTexture-&gt;SetRenderer(m_pRenderer);

        m_mapTexture[name] = pTexture;
    }

    if (!pTexture-&gt;LoadFromFile(filename))
        return nullptr;

    return pTexture;
}

void CTextureManager::SetRenderer(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}
The interface is badly designed:


Are you allowed to pass nullptr here?     
Is the object taking ownership of the pointer?

CTextureManager(SDL_Renderer *pRenderer);

Are you allowed to change the renderer?
Are you allowed to replace it with nullptr?
Is the ownership being passed to the CTextureManager object?

void SetRenderer(SDL_Renderer *pRenderer);

Passing name and filename by value. This is causing a copy. This is inefficient and not required. Pass by const reference.
The returned value:
Is it allowed to be nullptr?
Who owns the pointer?

CTexture *LoadFromFile(std::string name, std::string filename);

Another pass by value.
What does the return value indicate?

bool UnLoad(std::string name);

Pass by value again.
Who owns the returned object?

CTexture *GetTexture(std::string name);



You are storing pointers.

    std::map&lt;std::string, CTexture *&gt; m_mapTexture;
    SDL_Renderer *m_pRenderer;


Which means your class is doing both resource management and business logic. A class should do one or the other (search for Separation of Concerns). Split your resource management into its own class. Then use this inside your texture manager.

Ownership semantics.

Your Major problem is you are not correctly indicating the ownership semantics of the objects.

Ownership is all about who is responsible for deleting a dynamically allocated object and is one of the core principles of C++ (the one that raises C++ above C in terms of memory management). If you don't define these semantics well in the interface then people have to actually dig into the implementation to understand how the ownership is working so that they can correctly write their code.

If people are digging into your code to understand the ownership semantics they are now depending on implementation details of your code. This makes your code very brittle to any future changes and tightly couples any code using your class.


Return by reference to indicate that ownership is not being transferred.  
Pass by reference to indicate that you are not accepting ownership.  
Use smart pointers to pass unique/shared ownership objects around.  
Never pass by pointer across a public interface

its fine to use internally but it should not be part of your public interface to other code.



Prefer to use the constructors initialization list.

TextureManager::CTextureManager(SDL_Renderer *pRenderer)
{
    m_pRenderer = pRenderer;
}

// When you have members that have constructors.
// These will be called before the body of the constructor is
// entered.
//
// By initializing in the body of the code you
// are first constructing the object into the default state
// the updating that state. It is better to call the constructor
// directly with the appropriate parameters.
//
// Also reference must be done in the initialization list.
// So prefer to do all initialization in the initialization list
// for consistency.
TextureManager::CTextureManager(SDL_Renderer *pRenderer)
    : m_pRenderer(pRenderer)
{}


Destructor

You destructor is not virtual.

CTextureManager::~CTextureManager()


This means you are not expecting to have any sub types of the object. Which is fine. But you should keep it in mind.

ERROR

    if (m_pFont != nullptr)
    {
        TTF_CloseFont(m_pFont);
    }


m_pFont is not a member of the class. So I expect this would not compile.

Clear All

    for (std::map&lt;std::string, CTexture *&gt;::iterator it = m_mapTexture.begin(); it != m_mapTexture.end(); ++it)

   // Prefer to use `auto` for types you don't care about.
   // Also prefer to use the std::begin() and std::end() rather than
   // calling the methods directly. This will allow you to change the
   // container type in the future without having to change the code.
    for (auto it = std::begin(m_mapTexture); it != std::end(m_mapTexture); ++it)

   // Even better would be to use the new range based for
   for(auto&amp; value: m_mapTexture)

   // Or potentially an algorithm:
   std::for_each(td::begin(m_mapTexture), std::end(m_mapTexture), /* ACTION */);


Don't see any need to set this to nullptr:

        (it-&gt;second) = nullptr;


When you are finished you clear the container. So the values no longer exist.

GetTexture

On std::map the operator[] will insert a value into the container if it does not exist. I don't think that is what you actually want.

    return m_mapTexture[name];


UnLoad

Again with the insert.

    CTexture *pTempTexture = m_mapTexture[name];


Don't see the need for this test.

    if (!pTempTexture)
        return false;


Deleting a nullptr is fine.

    delete pTempTexture;


This does nothing useful (as it is about to go out of scope).

    pTempTexture = nullptr;


Since you inserted the name you may want to always delete it (even if the pointer is null).

    m_mapTexture.erase(name);


Just return the pointer here:

    return true;


The pointer will get converted to the correct bool. If you found nothing then it will be false. If you found something it will be true.    

LoadFromFile

So your object is always a CTexture never anything derived from it. 

        pTexture = new CTexture();


Since your texture is a single type I don't see the need to store pointers in the container. Just store the object directly.

Seems like the only reason for the texture manager to have a renderer is to make this call. Seems strange. Why not pass the renderer into the LoadFromFile() method?

        pTexture-&gt;SetRenderer(m_pRenderer);


You have just dynamically allocated and placed into your structure a CTecture. You may return a nullptr now. But a call to GetTexture() is going to return the previously created texture object (that failed to load). On a load failure you may want to remove this object from the structure.

    if (!pTexture-&gt;LoadFromFile(filename))
        return nullptr;


This is how I would do it:

// I am guessing a bit because I have not read the SDL documentation.
struct SLDTextureDeleter
{
    void operator()(SDL_Texture* p){SDL_DestroyTexture(p);}
};
class CTextureManager;
class CTexture
{
        // Constructor is private so it can only be used by CTextureManager
        friend class CTextureManager;
        CTexture(std::string const&amp; filename, CRenderer&amp; renderer)
        {
            // Create Texture and load from File.
            // Failure to load leaves the texture pointer as nullptr.
            // The TextureManager tests this by calling ok() to validate
            // that the texture was loaded correctly.

        }
        bool ok() const {return texture.get();}
    public:
        // Allow texture moving
        CTexture(CTexture&amp;&amp; move)
            : texture(std::move(move.texture))
        {}
        CTexture&amp; operator=(CTexture&amp;&amp; move)
        {
            using std::swap;
            swap(texture, move.texture);
        }
        // But disable Copy (as it is manager by CTextureManager)
        CTexture(CTexture const&amp;)            = delete;
        CTexture&amp; operator=(CTexture const&amp;) = delete;
    private:
        std::auto_ptr&lt;SDL_Texture, SLDTextureDeleter&gt;   texture;
};
class CTextureManager
{
  public:
    bool      loadFromFile(std::string const&amp; name, std::string const&amp; filename, CRenderer&amp; renderer);
    bool      hasTexture(std::string const&amp; name) const;    
    CTexture&amp; getTexture(std::string const&amp; name) const;

    void clearAll();
    void unLoad(std::string const&amp; name);    

  private:
    std::map&lt;std::string, CTexture&gt; mapTextures;
};

bool CTextureManager::loadFromFile(std::string const&amp; name, std::string const&amp; filename, CRenderer&amp; renderer)
{
    CTexture  tmp(filename, renderer);
    bool result = tmp.ok();
    if (result)
    {
        mapTextures.emplace(name, std::move(tmp));
    }
    return result;
}
bool CTextureManager::hasTexture(std::string const&amp; name) const
{
    auto find =   mapTextures.find(name);
    return find != mapTextures.end();
}
CTexture&amp; CTextureManager::getTexture(std::string const&amp; name) const;
{
    // Note: UB if loadFromFile() failed to return true.
    //          or if hasTexture() returned false.
    //          user is supposed to check before use.
    auto find =   mapTextures.find(name);
    return find-&gt;second;
}

void CTextureManager::clearAll()
{
    mapTextures.clear();
}
void CTextureManager::unLoad(std::string const&amp; name)
{
    mapTextures.erase(name);
}