SRP Violation – How to Avoid Violating the SRP in Caching Management Class

ccachingdatabaseobject-oriented-designsingle-responsibility

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?

Best Answer

If this class really was as trivial as it appears to be, then there would be no need to worry about violating the SRP. So what if a 3-line function has 2 lines doing one thing, and another 1 line doing another thing? Yes, this trivial function violates the SRP, and so what? Who cares? The violation of the SRP starts becoming a problem when things get more complicated.

Your problem in this particular case most probably stems from the fact that the class is more complicated than the few lines that you have shown us.

Specifically, the problem most probably lies in the fact that this class not only manages the cache, but also probably contains the implementation of the GetFluffiesFromDb() method. So, the violation of the SRP is in the class, not in the few trivial methods that are shown in the code that you posted.

So, here is a suggestion on how to handle all kinds of cases that fall within this general category, with the help of the Decorator Pattern.

/// 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 it is used as follows:

FluffiesProvider provider = new DatabaseFluffiesProvider();
provider = new CachingFluffiesProvider( provider );
...go ahead and use provider...

Note how CachingFluffiesProvider.GetFluffies() is not afraid to contain the code that does the time checking and updating, because that's trivial stuff. What this mechanism does is to address and handle the SRP at the system design level, where it matters, not at the level of tiny individual methods, where it does not matter anyway.

Related Topic