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?

migrated from codereview.stackexchange.com Nov 21 '15 at 16:31

This question came from our site for peer programmer code reviews.

  • 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. – 200_success Nov 21 '15 at 16:29
  • @200_success thank you, and sorry, I thought this would be adequate for CR – raven Nov 21 '15 at 19:32
  • 1
    So fluffy! – Mathieu Guindon Nov 22 '15 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. – whatsisname Nov 22 '15 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 '15 at 1:13
23

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.

  • 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. – Roman Reiner Nov 21 '15 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 '15 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 '15 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 '15 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 '15 at 15:46
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
    I like the simplicity in this. – raven Nov 22 '15 at 14:20
6

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;
}
  • While this is a pretty good first answer, please keep in mind that the "result" code is often a good addition. – Nic Hartley Nov 21 '15 at 15:38
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.

  • 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 '15 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 '15 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? – Dieter Meemken Nov 22 '15 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. – kevin cline Nov 22 '15 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. – kevin cline Nov 22 '15 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
    }
}
  • 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 '15 at 14:21

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.