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.
