Refactoring Exercise: The Single Responsibility Principle vs Needless Complexity

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.

Published by

Jan Van Ryswyck

Hi, thank you for visiting my blog and reading all the crap that I'm posting here. I'm a senior software engineer at SD WORX. Developing software is one of my greatest passions in life, and I enjoy doing it every single day. I've got three kids (Len, Lisa & Laura) who constantly remind me that there is more in life than just programming all day. They are the greatest kids in the whole world. And last but not least, there's my girlfriend who is my inspiration in life. You can always contact me at jan_dot_van_dot_ryswyck at gmail.com

17 thoughts on “Refactoring Exercise: The Single Responsibility Principle vs Needless Complexity”

  1. 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?

  2. 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?

  3. 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?

Comments are closed.