Single-Responsibility Versus Needless Complexity

At Pablo’s Day of TDD, we were discussing the Single-Responsibility Principle (SRP) while working through one of the labs and a question came up about a piece of code. The code in question looked something like the following (warning: this is over simplified code to show a point):

public bool Login(string username, string password)
{
    var user = userRepo.GetUserByUsername(username);

    if(user == null)
        return false;

    if (loginValidator.IsValid(user, password))
        return true;

    user.FailedLoginAttempts++;

    if (user.FailedLoginAttempts >= 3)
        user.LockedOut = true;

    return false;
}

This was from the a LoginService class and this was its only method. The question was whether or not this violates SRP because it’s in charge of incrementing FailLoginAttempts as well as locking the user out after 3 failed attempts. I believe we answered the question with a “depends”, but it bothered me that we didn’t have a better answer. Personally, I wouldn’t have busted this up into another class, but I didn’t have a good argument to stand on.

Today I went searching through Agile Principle, Patterns, and Practices in C# looking for a better answer. In the chapter on SRP, the book gives an example of an interface of a modem that can Dial/Hang-up and Send/Receive. The former is connection management and the later is data communication. The book asks the question as to whether or not these responsibilities should be separated. The answer is

“That depends on how the application is changing.”

It then gives an example to how a change might violate SRP, but then states:

“If, on the other hand,  the application is not changing in ways that cause the two responsibilities to change at different times, there is no need to separate them. Indeed, separating them would smell of needless complexity.”

Ah, there’s the backup wisdom that I needed to validate my gut feeling. Here’s one final quote from the book:

“There is a corollary here. An axis of change is an axis of change only if the changes occur. It is not wise to apply SRP (or any other principle, for that matter) if there is no symptom.”

I think applying SRP is about applying good judgement. You certainly don’t want to wait until you have to make a change before you think about SRP, but you don’t want to over do it either and end up with classes with one method with each having only a couple lines of code.

Technorati Tags:

Related Articles:

Post Footer automatically generated by Add Post Footer Plugin for wordpress.

About Ray Houston

Ray is a software development leader and architect with 20 years hands-on experience. He enjoys finding elegant solutions to complex problems and delivering real value to customers. Ray is a speaker and contributor to community events.
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.

7 Responses to Single-Responsibility Versus Needless Complexity

  1. Chad Myers says:

    Whomever asked about SRP had good instincts, though. That code does stick out like a sore thumb.

    I’d say the second any change came up that involved any more complexity in how failed login attempts are calculated or reset, refactor that into a separate class somehow.

    Also, the hard-coded ’3′ might need to become configurable — also an excuse to refactor this part.

  2. Ray Houston says:

    @chad – yeah, I agree. The point I’m trying to make here is not to abstract just to be abstract. If you do, you’ll have a lot of complexity that just ends up being waste. I’ve been down that road a time or two.

  3. Sean Scally says:

    Good points made there.

    Actually, Uncle Bob’s PPP book discusses this very topic in greater detail in the Agile Design / OCP chapters, i believe, under the topic of “taking the first bullet”.

    http://anydiem.com/2008/10/05/needless-complexity-and-taking-the-first-bullet/

  4. Chris says:

    Yeah – what sean said

    There is a quote along the lines of “being made a fool of the first time is OK” subsequent time not so

    i.e. wait until a scenario occurs that suggests you need to add in some extra flexibility rather than assume it needs it upfront

  5. Ryan says:

    Funny, I was having a very similar conversation with a co-worker late last week. Mike is an extremely smart developer and adheres to SOLID practices without having to think about it. He was very proud of a solution to a particularly nasty problem we’ve had recently, but when he presented his solution to the more junior developers on the team, it was met with a deafening silence. While his solution was EXTREMELY flexible, the junior guys just didn’t get it due to it’s complexity. There were simply too many relationships for the young guys to follow and they quickly became frustrated. What we had was a deer in the headlights scenario.

    Now, we’re working on bringing everyone’s knowledge of good OO design up a notch or two, but it’s going to take time. For now, we know for certain that some parts of our application are going to change, so Mike and I worked for a while to come up with a good solution that just applied to those parts of the app. We cut the object diagram down by about 80% and STILL had a flexible solution that solved all the problems that we knew (or suspected) would change. When we went back to the developers, they were MUCH more receptive.

    What gets me is that Mike knows better. We’ve been doing a great job with agile practices for about six months now, and Mike knows (or should know) not to design his application for some far off, grandiose idea of what could be a year or two down the road. Chances are that 95% of the flexibility provided by the initial design would never be needed.

  6. I agree with you, but in the example above IMHO the SRP should be applied

    We can split the method in three distinct sections with three distinct purposes

    public bool Login(string username, string password) {
    // find the user object
    var user = userRepo.GetUserByUsername(username);
    if(user == null)
    return false;

    // validate the user login
    if (loginValidator.IsValid(user, password))
    return true;

    // apply the login failures’s business logic
    user.FailedLoginAttempts++;
    if (user.FailedLoginAttempts >= 3)
    user.LockedOut = true;

    return false;
    }

    The user object can be passed as an argument (so that the Login method can be more easely tested without mock/stub), the caller can retrieve the user object from the repo, and the repo can return a NotFoundUser (null object pattern) which should fail all the login validation

    The last section smells like “Feature Envy”, maybe the ‘user’ should deal with the login failures’s business logic

    public bool Login(User user, string password) {
    if (loginValidator.IsValid(user, password)) {
    return true;
    }
    user.FailedLogin();
    return false;
    }

    Last steps:
    - We can/should inline loginValidator.isValid or move it to this class
    - ‘Login’ seems to me more a ‘do’ method then a ‘query’ method

    In the end, something like this

    public void Login(User user, string password) {
    if (IsNotTheUserPassword(user, password)) {
    user.FailedLogin();
    throw new LoginFailedException(user, password);
    }
    }

    Sorry for my bad english

Leave a Reply

Your email address will not be published. Required fields are marked *

*

You may use these HTML tags and attributes: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <cite> <code> <del datetime=""> <em> <i> <q cite=""> <strike> <strong>