Font manager/loader class written in C++ (uses unordered_map)

Question

This font manager is part of a game engine. It's supposed to mostly load fonts at the start, so I'm more worried about reading performance than loading performance. If there's any way of improving loading performance as well I'm happy to hear it.

• How would I optimize this font manager or managers in general, and what is the best practices for doing so?
• Is there any way of improving the use of the unordered_map container?
• I'm also trying to write more modern C++ code. Is there anything that could be improved on that front as well?

Header file:

class FontManager {
public:
    static FontManager &Instance() {
        static FontManager singleton;
        return singleton;
    }

    const Font *LoadFont(const std::string fileName);
    void BatchLoadFonts(const std::vector<std::string>& infos);

    const Font *GetFont(const std::string& fileName);
private:
    FontManager() { }
    ~FontManager();
private:
    std::unordered_map<std::string, const Font*> m_Fonts;
};

.cpp file:

const Font *FontManager::LoadFont(const std::string fileName) {
    if (m_Fonts.count(fileName))
        return m_Fonts.at(fileName);

    m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
    return m_Fonts.at(fileName);
}

void FontManager::BatchLoadFonts(const std::vector<std::string>& infos) {
    for (auto info : infos) {
        LoadFont(info);
    }
}

const Font *FontManager::GetFont(const std::string &fileName) {
    if (m_Fonts.count(fileName)) {
        return m_Fonts.at(fileName);
    }

    LoadFont(fileName);
    return m_Fonts.at(fileName);
}

FontManager::~FontManager() {
    for (auto const& f : m_Fonts) {
        delete f.second;
    }
}

Answer 1

Relationship between batch and single-call api

Typically, when a manager class like like has both a single and a batch API call, the general function is the batch API, and the special case is the single-case. so it's the single-call API that should be calling the batch API, or the batch api should not exist at all.

The reasoning is that normally, the batch API provides some kind of advantage on top of just calling the single call in a loop. If there is no advantage to be gained, then there should only be the single resource call function, and let your user write a loop around it if they need to call it a bunch of time.

You want to keep your API surface as small as possible.

Smart pointers

Unless you are dealing with some very legacy system, you should have access to C++11. use std::unique_ptr<> instead of raw pointers, it'll massively reduce the amount of possible errors, as well as the amount of code you have to write.

Avoid double lookups:

if (m_Fonts.count(fileName))
    return m_Fonts.at(fileName);

// vs

auto found = m_Fonts.find(fileName);
if(found != m_Fonts.end()) 
    return found->second;

The second block of code only does a single lookup through the container.

similarly:

LoadFont(fileName);
return m_Fonts.at(fileName);

// vs

return LoadFont(fileName);

and

m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
return m_Fonts.at(fileName);

// vs

auto emplaced = m_Fonts.emplace(fileName, new Font(FONT_FOLDER_PATH + fileName));
assert(emplaced.second);
return emplaced.first->second;

removes extra lookups.

avoid Singleton instantiation in the header file

This is a little subtle, but the definition of FontManager::instance belongs in the cpp file. This is important because if DLLs are ever involved, you are going to have a bad time otherwise.