Blog

  • Home /
  • Blog /
  • Refactoring Exercise: The Single Responsibility Principle vs Needless Complexity

Refactoring Exercise: The Single Responsibility Principle vs Needless Complexity

October 6, 2008

Ray Houston has written this post on his blog named Single-Responsibility Versus Needless Complexity. His post contains the following code sample of which he suspects that it possibly violates the Single Responsibility Principle:

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

Because I’m still sitting at home slowly recovering from surgery (almost a month now), and because I’m terribly bored by now, I thought I’d just try to refactor this code. The first step I took was to isolate the validation of the user in a separate private method like so:

private bool IsValid(User user, String password)
{
    if(user == null)
        return false;
    
    return loginValidator.IsValid(user, password);
}

This slightly simplifies the code of the Login method:

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

    user.FailedLoginAttempts++;

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

    return false;
}

Next thing I noticed was the use of the FailedLoginAttempts and the LockedOut properties, which appear to be part of the User class. This is something I talked about in Properties - A False Sense of Encapsulation. Let us apply Tell, Don’t Ask and move this code into the User class like so: 

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

    user.FailedLoginAttempts++;

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

    return false;
}

This eliminates the need of the properties I just mentioned, which expose private data of the User class (at least for the Login story). The code of the Login method now becomes fairly small:

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

    user.FailedLoginAttempts++;

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

    return false;
}

Besides good judgement, SRP is also about organizing complexity so that other developers/readers know where to look for it.

Now get those torches lit and flame away.

Profile picture of Jan Van Ryswyck

Jan Van Ryswyck

Thank you for visiting my blog. I’m a professional software developer since Y2K. A blogger since Y2K+5. Curator of the Awesome Talks list. Past organizer of the European Virtual ALT.NET meetings. Thinking and learning about all kinds of technologies since forever.

Comments

About

Thank you for visiting my website. I’m a professional software developer since Y2K. A blogger since Y2K+5. Curator of the Awesome Talks list. Past organizer of the European Virtual ALT.NET meetings. Thinking and learning about all kinds of technologies since forever.

Contact information

(+32) 496 38 00 82

infonull@nullprincipal-itnull.be