12

Note: The code sample is written in c#, but that shouldn't matter. I've put c# as a tag because I can't find a more appropiate one. This is about the code structure.

I'm reading Clean Code and trying to become a better programmer.

I often find myself struggling to follow the Single Responsibility Principle (classes and functions should do only one thing), specially in functions. Maybe my problem is that "one thing" is not well-defined, but still...

An example: I have a list of Fluffies in a database. We don't care what a Fluffy is. I want a class to recover fluffies. However, fluffies can change according to some logic. Depending on some logic, this class will return the data from cache or get the latest from the database. We could say that it manages fluffies, and that is one thing. To make it simple, let's say loaded data is good for an hour, and then it must be reloaded.

class FluffiesManager
{
    private Fluffies m_Cache;
    private DateTime m_NextReload = DateTime.MinValue;
    // ...
    public Fluffies GetFluffies()
    {
        if (NeedsReload())
            LoadFluffies();

        return m_Cache;
    }

    private NeedsReload()
    {
        return (m_NextReload < DateTime.Now);
    }

    private void LoadFluffies()
    {
        GetFluffiesFromDb();
        UpdateNextLoad();
    }

    private void UpdateNextLoad()
    {
        m_NextReload = DatTime.Now + TimeSpan.FromHours(1);
    }
    // ...
}

GetFluffies() seems ok to me. The user asks for some fluffies, we provide them. Going to recover them from the DB if needed, but that could be considered a part of getting the fluffies (of course, that's somewhat subjective).

NeedsReload() seems right, too. Checks if we need to reload the fluffies. UpdateNextLoad is fine. Updates the time for the next reload. that's definitely one single thing.

However, I feel what LoadFluffies() do can't be described as one single thing. It's getting the data from the Database, and it's scheduling the next reload. It's hard to argue that calculating the time for the next reload is part of getting the data. However, I can't find a better way to do it (renaming the function to LoadFluffiesAndScheduleNextLoad may be better, but it just makes the problem more obvious).

Is there an elegant solution to really write this class according to the SRP? Am I being too pedantic?

Or maybe my class isn't really doing just one thing?

5
  • 3
    Based on "written in C#, but that shouldn't matter", "This is about code structure", "An example: … We don't care what a Fluffy is", "To make it simple, let's say…", this isn't a request for a code review, but a question about a general programming principle. Nov 21, 2015 at 16:29
  • @200_success thank you, and sorry, I thought this would be adequate for CR
    – raven
    Nov 21, 2015 at 19:32
  • 1
    So fluffy! Nov 22, 2015 at 2:56
  • 2
    In the future you'd be better off with "widget" instead of fluffy for future similar questions, as a widget is understood to be a non-particular stand in for examples. Nov 22, 2015 at 5:35
  • 1
    I know it's only example code, but use DateTime.UtcNow so you avoid daylight savings changeovers, or even a change in the current timezone.
    – Mark Hurd
    Nov 25, 2015 at 1:13

6 Answers 6

26

One general-purpose mechanism for handling a broad range of cases where we want to add value without violating the Single Responsibility Principle is the Decorator Pattern.

This mechanism is suitable when the adding of value can be done without changing the existing interface, which is typically what caches do.

The beautiful thing with this mechanism is that it takes the handling out of the code and into the design.

Specifically:

  • We begin by declaring an interface for what we want to do; in your case, a FluffiesProvider.
  • Then we write a class which implements FluffiesProvider and offers the fundamental functionality, in your case a DatabaseFluffiesProvider which reads the fluffies from the database without worrying at all about caching.
  • Then we write a decorator of FluffiesProvider which does nothing but caching and has no idea where the fluffies come from.
  • Finally, we wire them together as one, so the final FluffiesProvider that we end up with is a cached database fluffies provider.

Here is some example code:

/// Provides Fluffies.
interface FluffiesProvider
{
    Fluffies GetFluffies();
}

/// Implements FluffiesProvider using a database.
class DatabaseFluffiesProvider : FluffiesProvider
{
    public override Fluffies GetFluffies()
    {
        ... load fluffies from DB ...
        (the entire implementation of "GetFluffiesFromDb()" goes here.)
    }
}

/// Decorates FluffiesProvider to add caching.
class CachingFluffiesProvider : FluffiesProvider
{
    private FluffiesProvider decoree;
    private DateTime m_NextReload = DateTime.MinValue;
    private Fluffies m_Cache;

    public CachingFluffiesProvider( FluffiesProvider decoree )
    {
        Assert( decoree != null );
        this.decoree = decoree;
    }

    public override Fluffies GetFluffies()
    {
        if( DateTime.Now >= m_NextReload ) 
        {
             m_Cache = decoree.GetFluffies();
             m_NextReload = DatTime.Now + TimeSpan.FromHours(1);
        }
        return m_Cache;
    }
}

and here is the instantiation and wiring together of the classes:

FluffiesProvider provider = new DatabaseFluffiesProvider();
provider = new CachingFluffiesProvider( provider );
...go ahead and use provider...
7
  • 1
    +1 for recognizing that fluffies, caching and data base access are actually three responsibilities. You could even try to make the FluffiesProvider interface and the decorators generic (IProvider<Fluffy>, ...) but this might be YAGNI. Nov 21, 2015 at 19:29
  • Honestly, if there is only one type of cache and it always pulls objects from the database, this is IMHO heavily overdesigned (even if the "real" class might be more complex as we can see in the example). Abstraction just for the sake of abstraction does not make code cleaner or more maintainable.
    – Doc Brown
    Nov 22, 2015 at 11:45
  • @DocBrown problem is the lack of context to the question. I like this answer beacues it shows a way which I have used time and time again in larger applications and because it's easy to write tests against, I also like my answer because it's only a small change and yields something clear without any overdesigning so as it currently stands, without context pretty much all answers here are good :]
    – stijn
    Nov 22, 2015 at 12:00
  • 1
    FWIW, the class I had in mind when I asked the question is more complicated than FluffiesManager, but not overly so. Some 200 lines, maybe. I haven't asked this question because I have found any problem with my design (yet?), just because I couldn't find a way to strictly comply with the SRP, and that could be a problem in more complex cases. So, the lack of context is somewhat intended. I think this answer is great.
    – raven
    Nov 22, 2015 at 14:19
  • 2
    @stijn: well, I think your answer is heavily undervoted. Instead of adding unnecessary abstraction, you just cut and name the responsibilities differently, which should be always the first choice before piling three layers of inheritance to such a simple problem.
    – Doc Brown
    Nov 22, 2015 at 15:46
7

I believe your class is doing one thing; it's a data cache with a timeout. LoadFluffies seems like a useless abstraction unless you call it from multiple places. I think it would be better to take the two lines from LoadFluffies and put them in the NeedsReload conditional in GetFluffies. This would make the implementation of GetFluffies a lot more obvious and is still clean code, as you're composing single responsibility subroutines to accomplish a single goal, a cached retrieval of data from the db. Below is the updated get fluffies method.

public Fluffies GetFluffies()
{
    if (NeedsReload()) {
        GetFluffiesFromDb();
        UpdateNextLoad();
    }

    return m_Cache;
}
1
  • While this is a pretty good first answer, please keep in mind that the "result" code is often a good addition.
    – Nic
    Nov 21, 2015 at 15:38
6

Your class itself seems fine to me, but you're right in that LoadFluffies() does not exactly what the name advertises. One simple solution would be to change the name and move the explicit reloading out of GetFluffies, into a function with an appropriate description. Something like

public Fluffies GetFluffies()
{
  MakeSureTheFluffyCacheIsUpToDate();
  return m_Cache;
}

private void MakeSureTheFluffyCacheIsUpToDate()
{
  if( !NeedsReload )
    return;
  GetFluffiesFromDb();
  SetNextReloadTime();
}

looks clean to me (also because as like Patrick says: it's composed of other tiny SRP-obedient functions), and especially also clear which is sometimes just as important.

1
  • 1
    I like the simplicity in this.
    – raven
    Nov 22, 2015 at 14:20
4

Your instincts are correct. Your class, small though it may be, is doing too much. You should separate the timed refresh caching logic into a completely generic class. Then create a specific instance of that class for managing Fluffies, something like this (not compiled, working code is left as an exercise for the reader):

public class TimedRefreshCache<T> {
    T m_Value;
    DateTime m_NextLoadTime;
    Func<T> m_producer();
    public CacheManager(Func<T> T producer, Interval timeBetweenLoads) {
          m_nextLoadTime = INFINITE_PAST;
          m_producer = producer;
    }
    public T Value {
        get {
            if (m_NextLoadTime < DateTime.Now) {
                m_Value = m_Producer();
                m_NextLoadTime = ...;
            }
            return m_Value;
        }
    }
}

public class FluffyCache {
    private TimedRefreshCache m_Cache 
        = new TimedRefreshCache<Fluffy>(GetFluffiesFromDb, interval);
    private Fluffy GetFluffiesFromDb() { ... }
    public Fluffy Value { get { return m_Cache.Value; } }
}

An added advantage is that it is now very easy to test TimedRefreshCache.

5
  • 1
    I agree that if the refresh logic gets more complicated than in the example, it might be a good idea to refactor it into a separate class. But I disagree that the class in the example, as it is, does too much.
    – Doc Brown
    Nov 22, 2015 at 11:50
  • @kevin, I'm not experienced in TDD. Could you elaborate on how would you test TimedRefreshCache? I don't see it as "very easy", but it could be my lack of expertise.
    – raven
    Nov 22, 2015 at 14:06
  • 1
    I personally don't like your answer because of it's complexity. It is very generic and very abstract and may be best in more complicated situations. But in this simple case it is 'simply to much'. Please take a look at stijn's answer. What a nice, short and readable answer. Everybody will understand it imediatly. What do you think? Nov 22, 2015 at 21:16
  • 1
    @raven You can test TimedRefreshCache by using a short interval (like 100ms) and a very simple producer (like DateTime.Now). Every 100 ms the cache will produce a new value, in between it will return the previous value. Nov 22, 2015 at 21:18
  • 1
    @DocBrown: The problem is that as written it is untestable. The timing logic (testable) is coupled with the database logic, which much then be mocked. Once a seam is created to mock the database call, you are 95% of the way to the generic solution. I have found that building these little classes usually pays off because they end up being reused more than expected. Nov 22, 2015 at 21:25
1

Your class is fine, SRP is about a class not a function, the whole class is responsible for providing the "Fluffies" from the "Data Source" so you are free in internal implementation.

If you wish to expand the cahing mechanism you can create class respnsible for watching data source

public class ModelWatcher
{

    private static Dictionary<Type, DateTime> LastUpdate;

    public static bool IsUpToDate(Type entityType, DateTime lastRead) {
        if (LastUpdate.ContainsKey(entityType)) {
            return lastRead >= LastUpdate[entityType];
        }
        return true;
    }

    //call this method whenever insert/update changed to any entity
    private void OnDataSourceChanged(Type changedEntityType) {
        //update Date & Time
        LastUpdate[changedEntityType] = DateTime.Now;
    }
}
public class FluffyManager
{
    private DateTime LastRead = DateTime.MinValue;

    private List<Fluffy> list;



    public List<Fluffy> GetFluffies() {

        //if first read or not uptodated
        if (list==null || !ModelWatcher.IsUpToDate(typeof(Fluffy),LastRead)) {
            list = ReadFluffies();
        }
        return list;
    }
    private List<Fluffy> ReadFluffies() { 
    //read code
    }
}
2
  • According to Uncle Bob: FUNCTIONS SHOULD DO ONE THING. THEY SHOULD DO IT WELL. THEY SHOULD DO IT ONLY. Clean Code p.35.
    – raven
    Nov 22, 2015 at 14:21
  • According to me: Never trust anyone calling themselves "uncle".
    – gnasher729
    Nov 24 at 8:47
0

"Single responsibility principle" is the one design pattern with the worst misinterpretations, causing the worst damage if misapplied.

As far as any user of your class is concerned, it does one thing: It returns a "Fluffies" object that is more or less up-to-date. That's it's responsibility, and that's what it does. (It could also be part of the interface to state that the call is almost always very cheap, cheap enough to not try to avoid it, and sometimes it is expensive, but at those times you really wanted the call).

Of course you can write two public methods, one that reloads from the database when too much time was spent since the last reload, and one that returns the last loaded fluffier. Or three public methods, one that checks if too much time was spent since the last reload, one that reloads the data, and one that returns the last load Fluffies.

That's of no benefit to the caller whatsoever, because they have to make either two calls, or three calls where they need to decide when to reload the data, just making things more complicated. It just exposes what should be internal implementation details of the class.

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.