How to avoid violating the SRP in a class to manage caching?

Question

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?

Answer 1

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

Answer 2

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;
}

Answer 3

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.

Answer 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.

Answer 5

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
    }
}

Answer 6

"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.