Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

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

share|improve this question
1  
Reminds me of Martin Fowler's ActiveRecord vs DataMapper. –  User Jul 22 '11 at 22:17
    
Provide convenience (your current design) in the outermost, user-facing interface. Use SRP internally so that it will ease your future implementation changes. I can think of Font loader embellishments that skips italic and bold; that only loads the Unicode BMP, etc. –  rwong Jul 31 '11 at 17:52
    
    
@rwong I know of that presentation, I had a bookmark to it (video). :) But I don't understand what you're saying in your other comment... –  Paul Jul 31 '11 at 18:04
1  
@rwong Isn't it already a one liner? You only need one line, whether you load a Font directly or through the ResourceManager. And what stops me from reimplementing the RM if users complain? –  Paul Jul 31 '11 at 18:25
show 1 more comment

2 Answers

up vote 6 down vote accepted

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.

share|improve this answer
1  
Ah, I see: if I add one more way to load a font, I'll need to add one more acquire function to the manager and one more load function to the resource. Indeed, that's one disadvantage. Depending on what this new source might be, however, you'll probably have to handle the data differently (TTFs are one thing, font sprites are another), so you can't really predict how flexible a certain design is going to be. I do see your point, though. –  Paul Jun 6 '11 at 10:24
    
Yeah, like I said, my C++ skills are pretty rusty so I struggled to come up with a viable demonstration of the issue, I agree about the flexibility thing. It really depends what you're going for with your code, like I said, I think your original code is perfectly reasonably solution to the problem. –  Ed Woodcock Jun 6 '11 at 10:26
    
Great question and great answer, and the best thing is that multiple developers can learn from it. This is why I love hanging out here :). Oh and so my comment isn't completely redundant, SRP can be a little bit tricky because you do have to have to ask yourself 'what if', which can seem counter to: 'premature optimisation is the root of all evil' or 'YAGNI' philosophies. There's never a black and white answer! –  Martijn Verburg Jun 6 '11 at 11:51
add comment

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

share|improve this answer
    
The load functions are accessible from two constructors, and free is called in the destructor -- I just didn't show that here. I find it convenient to be able to load the font directly from a file, instead of first opening the file, writing the data to a buffer and then passing that to Font. Sometimes I also need to load from a buffer, though, which is why I have both methods. –  Paul Jun 6 '11 at 14:26
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.