Pergunta

In the past months I've asked for people here on SE and on other sites offer me some constructive criticism regarding my code. There's one thing that kept popping out almost every time and I still don't agree with that recommendation; :P I'd like to discuss it here and maybe things will become clearer to me.

It's regarding the single-responsibility principle (SRP). Basically, I have a data class, Font, that not only holds functions for manipulating the data, but also for loading it. I'm told the two should be separate, that loading functions should be placed inside a factory class; I think this is a mis-interpretation of the SRP...

A Fragment from My Font Class

class Font
{
  public:
    bool isLoaded() const;
    void loadFromFile(const std::string& file);
    void loadFromMemory(const void* buffer, std::size_t size);
    void free();

    void some();
    void another();
};

Suggested Design

class Font
{
  public:
    void some();
    void another();
};


class FontFactory
{
  public:
    virtual std::unique_ptr<Font> createFromFile(...) = 0;
    virtual std::unique_ptr<Font> createFromMemory(...) = 0;
};

The suggested design supposedly follows the SRP, but I disagree — I think it goes too far. The Font class is not longer self-sufficient (it is useless without the factory), and FontFactory needs to know details about the implementation of the resource, which is probably done through friendship or public getters, which further expose the implementation of Font. I think this is rather a case of fragmented responsibility.

Here's why I think my approach is better:

  • Font is self-sufficient — Being self-sufficient, it's easier to understand and maintain. Also, you can use the class without having to include anything else. If, however, you find you need a more complex management of resources (a factory) you can easily do that as well (later I'll talk about my own factory, ResourceManager<Font>).

  • Follows the standard library — I believe user-defined types should try as much as possible to copy the behavior of the standard types in that respective language. The std::fstream is self-sufficient and it provides functions like open and close. Following the standard library means there's no need to spend effort learning yet another way of doing things. Besides, generally speaking, the C++ standard committee probably knows more about design than anyone here, so if ever in doubt, copy what they do.

  • Testability — Something goes wrong, where could the problem be? — Is it the way Font handles its data or the way FontFactory loaded the data? You don't really know. Having the classes be self-sufficient reduces this problem: you can test Font in isolation. If you then have to test the factory and you know Font works fine, you'll also know that whenever a problem occurs it must be inside the factory.

  • It is context agnostic — (This intersects a bit with my first point.) Font does its thing and makes no assumptions about how you'll use it: you can use it any way you like. Forcing the user to use a factory increases coupling between classes.

I too Have a Factory

(Because the design of Font allows me to.)

Or rather more of a manager, not merely a factory... Font is self-sufficient so the manager doesn't need to know how to build one; instead the manager makes sure the same file or buffer isn't loaded into memory more than once. You could say a factory can do the same, but wouldn't that break the SRP? The factory would then not only have to build objects, but also manage them.

template<class T>
class ResourceManager
{
  public:
    ResourcePtr<T> acquire(const std::string& file);
    ResourcePtr<T> acquire(const void* buffer, std::size_t size);
};

Here's a demonstration of how the manager could be used. Notice that it's used basically exactly as a factory would.

void test(ResourceManager<Font>* rm)
{
    // The same file isn't loaded twice into memory.
    // I can still have as many Fonts using that file as I want, though.
    ResourcePtr<Font> font1 = rm->acquire("fonts/arial.ttf");
    ResourcePtr<Font> font2 = rm->acquire("fonts/arial.ttf");

    // Print something with the two fonts...
}

Bottom Line...

(It'd like to put a tl;dr here, but I can't think of one. :\ )
Well, there you have it, I've made my case as best as I could. Please post any counter-arguments you have and also any advantages that you think the suggested design has over my own design. Basically, try to show me that I'm wrong. :)

Nenhuma solução correta

Licenciado em: CC-BY-SA com atribuição
scroll top