5
\$\begingroup\$

It started with a simple log manager. Then I wanted to implement more features, for example printing the name of the component that send a message. Later on, I can think of giving each component that prints log messages it's own text color in the terminal and options to mute or solo single modules.

Therefore, the manager needs the identity of the component that called its methods. So I decided to wrap the manager with an instance for every component. It tunnels function calls to the manager together with the name of the component.

#include <iostream>
#include <string>
#include <memory>
#include <unordered_map>
using namespace std;

////////////////////////////////////////////////////////////////
// Manager declaration
////////////////////////////////////////////////////////////////

class manager {
public:
    class instance;

    instance &get_instance(string name);
    void print(string sender, string message);

private:
    unordered_map<string, unique_ptr<instance>> m_instances;
};

////////////////////////////////////////////////////////////////
// Instance declaration
////////////////////////////////////////////////////////////////

class manager::instance {
public:
    instance(string name, manager &manager);
    void print(string message);

private:
    string m_name;
    manager &m_manager;
};

////////////////////////////////////////////////////////////////
// Manager implementation
////////////////////////////////////////////////////////////////

manager::instance &manager::get_instance(string name) {
    if (m_instances.find(name) == m_instances.end())
        m_instances[name] = unique_ptr<instance>(new instance(name, *this));
    return *m_instances[name].get();
}

void manager::print(string sender, string message) {
    cout << sender << ": " << message << "." << endl;
}

////////////////////////////////////////////////////////////////
// Instance implementation
////////////////////////////////////////////////////////////////

manager::instance::instance(string name, manager &manager) : m_name(name),
    m_manager(manager) {}

void manager::instance::print(string message) {
    m_manager.print(m_name, message);
}

////////////////////////////////////////////////////////////////
// Main function
////////////////////////////////////////////////////////////////

int main() {
    manager m;
    m.get_instance("Window").print("Open up");
    m.get_instance("Asset").print("Load textures");
    return 0;
}

Here is a working example of my code. What do you think about the class structure and architecture? Is there an easier or more expressive way? I'd like to get feedback and improve the code structure before implementing other managers the same way.

\$\endgroup\$
3
  • \$\begingroup\$ Out of curiosity, is this from a game you are making? If so, what type of game? \$\endgroup\$
    – bazola
    Oct 9 '14 at 0:20
  • \$\begingroup\$ @bazola Yes, I just started rewriting my game engine. You can find the project on Github. \$\endgroup\$
    – danijar
    Oct 9 '14 at 1:46
  • \$\begingroup\$ Looks really cool! \$\endgroup\$
    – bazola
    Oct 9 '14 at 2:01
5
\$\begingroup\$

About the architecture and thinking exclusively on the given example application (logging), I believe manager does not need to track all manager::instance instances. Maybe in other applications this might be needed, but not in this one. Thus, I would just turn get_instance into a convenience method (make_instance) to create new manager::instances:

class manager {
public:
    class instance;

    std::unique_ptr<instance> make_instance(const std::string& name);
    void print(const std::string& sender, const std::string& message);
};

std::unique_ptr<manager::instance> manager::make_instance(const std::string& name) {
    return std::unique_ptr<instance>(new instance(name, *this));
}

If you plan using the manager like a singleton (having only one instance and everyone can access it), maybe the manager and manager::instance classes could be merge into a single one, where manager members would become static and manager::instance would become an actual instance of manager.

Please note that both simplifications above may not apply to other applications.

Now, the general code review comments (some of which were already applied on the snippet above):

The code is very well organized, very easy to read and understand. The coding style is consistent. I have only 3 suggestions for improvement:

Always pass std::string to a function as const std::string& instead of just std::string to avoid an unnecessary copy.

Avoid using namespace std;. If, for instance, you really want to use some std symbols unqualified, import them explicitly (like using std::string;). Also avoid using declarations on headers where they may affect user code (in global or namespace scope), otherwise it may break other's code when they include your header.

Also, use braces after if, while, do and for. Those kind of bugs (adding another indented line thinking it inside a block) are hard to spot.

\$\endgroup\$
2
  • \$\begingroup\$ Is the general idea to wrap the whole class good? Are there alternatives? You are right about make_instance. Actually, I just wanted to prevent two components from using the same name, but obviously the code fails at this. What do you think about using an unordered_set to store names already used for this? Also, thanks for the tip of passing strings by reference, I guess I review the rest of the application for that as well. \$\endgroup\$
    – danijar
    Oct 9 '14 at 11:40
  • \$\begingroup\$ If you need to prohibit duplicated names, unordered_set is a good choice IMO, but in an internal log manager, I don't think using the same name twice by mistake would ever happen. Maybe that's even useful in the case of multiple instances of a component. \$\endgroup\$ Oct 10 '14 at 21:59

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.