Refactoring Exercise: The Single Responsibility Principle vs Needless Complexity

October 6th, 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:

private const Int32 MaximumFailedLoginAttempts = 3 public void FailsToLogin() { this.failedLoginAttempts += 1; if(MaximumFailedLoginAttemptsExceeded()) LockOut(); } private Boolean MaximumFailedLoginAttemptsExceeded() { return this.failedLoginAttempts >= MaximumFailedLoginAttempts; } private void LockOut() { this.lockedOut = true; }

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(false == IsValid(user, password)) { user.FailedToLogin(); return false; } return true; }

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.

  • Pingback: Dew Drop - October 7, 2008 | Alvin Ashcraft's Morning Dew()

  • Pingback: Arjan`s World » LINKBLOG for October 7, 2008()

  • Eric

    So where are you persisting the number of failed login attempts, updating that the account was locked and possibly writing the failed attempt or lockout (or both) to a system log?

  • Eric

    Not to mention resetting the number of failed login attempts after a successful login?

    Also where are you checking to see if the account is locked to see if you should bother validating the username/password anyway?

  • http://mandersn.net Mark Anderson

    Likely another problem with refactoring in notepad:

    When you isolated the validation of the user in IsValid(), you lost the behavior that a null username fell out of the Login() method returning false. The behavior that you have is that if user is null, you are turning around and calling FailedToLogin() on the null user. Wouldn’t that cause a NPE?

  • http://elegantcode.com Jan Van Ryswyck

    @Mark: You are right on the NPE.

  • master plastic

    Absolutely unique and fine piece of information.
    pvc pakistan I’ve never spent that much time reading before but this is really awesome.. 
    pvc