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.
Very nice! That makes good sense to move that directly to the User class so that it doesn’t leak out.
Pfew! Glad you like it :-).
I’m not seeing where you passed the ‘password’ into the IsValid method. Originally, password was available in the scope of the all encompassing method. When you factored the validation logic out, you’d need to pass the password in as well as the user to IsValid().
Is that right, or did I miss something?
Really nitpicky point, but MaximumFailedLoginAttemptsExceeded is poorly named since it does a “>=” comparison. Should rename to MaximumFailedLoginAttemptsReached or something.
thanks. I’ve been struggling with the tell don’t ask principle. Your article cleared that up.
I like it too. Shouldn’t IsValid take a password parameter? Also, what’s the login validator for? Is that to provide app services like dealing with hashed password and salt?
I had a play with a couple of ideas, although I’m not sure I like either of them, but fun to throw em out there…
This one makes the login attempt an explicit class.
public bool Login(string username, string password)
{
var user = userRepo.GetByUsername(username);
if(user == null)
return false;
var attempt = new LoginAttempt(user, password);
return attempt.Succeeded;
}
Another one use the Null object pattern. This looks weird though, which is good enough reason not to use it?
public bool Login(string username, string password)
{
var user = userRepo.GetByUsername(username) ?? User.Null;
return new LoginAttempt(user, password).Succeeded;
}
You guys are right on the ‘password’ thing. Will correct it right away. That’s what you get when refactoring in Notepad2. No Resharper support.
@jdn: Your function name is a lot better. Thanks!
Given the fact that I forgot the password argument to the IsValid method, maybe its better to rename this method to ‘Authenticate’ or something?
A few random thoughts. (Just finished reading “Clean Code” as well, awesome book.)
Argument validation, generally I try to move this as deep as possible to avoid issues. loginValidator takes a user name & password. Should it ever validate a null (or empty) user name successfully? This would essentially get rid of IsValid().
This would change Login to:
public bool Login(string username, string password)
{
var user = userRepo.GetUserByUsername(username);
bool isValid = loginValidator.IsValid(user, password))
if ( !isValid) // or (false = isValid)
{
user.FailedToLogin();
}
return isValid;
}
generally I’m a stickler for 1-way-in, 1-way-out.
ack, scratch that. Checking the returned user reference for null, then validating the user against PW. ^_^ I shouldn’t read code before noon. *sigh*
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?
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?
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?
@Mark: You are right on the NPE.
Absolutely unique and fine piece of information.
pvc pakistan I’ve never spent that much time reading before but this is really awesome..
pvc