Single-responsibility and custom data types

Question

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. :)

Answer 1

There's nothing wrong with that code in my opinion, it does what you need it to in a sensible and reasonably easy to maintain manner.

However, the problem that you have with this code is that if you want it to do anything else you're going to have to change it all.

The point of the SRP is that if you have a single component 'CompA' that does algorithm A() and you need to change algorithm A() you shouldn't have to change 'CompB' as well.

My C++ skills are too rusty to suggest a decent scenario where you will need to change your font management solution, but the usual case I make is the idea of sliding in a caching layer. Ideally you don't want the thing that loads stuff to know about where it comes from, nor should the thing being loaded care where it comes from, because then making changes is simpler. It's all about maintainability.

One example might be you loading your font from a third source (say a character sprite image). In order to achieve this you will need to change your loader (to call the third method if the first two fail) and the Font class itself to implement this third call. Ideally, you'd just make another factory (SpriteFontFactory, or whatever), implement the same loadFont(...) method and stick it in a list of factories somewhere that can be used to load the font.

Answer 2

One thing that bugs me about your class is that you have loadFromMemory and loadFromFile methods. Ideally, you should have only loadFromMemory method; a font should not care how the data in memory came to be. Another thing is that you should use constructor/destructor instead of load and free methods. Thus, loadFromMemory would become Font(const void *buf, int len) and free() would become ~Font().