During a TDD/BDD training session with my team, today, we wrote the following code:
public void SaveRequested()
{ Employee employee = Department.CreateEmployee(firstName, lastName, email);
if (employee == null)
repository.Save(Department);
}
I ignored this when we first wrote it, as it was "technically correct", but later returned to this code as a point of refactoring for greater insight and expressiveness. When we came back to it, though, I started off by asking a series of questions:
- Why would that method return a null?
- Why would it not return a null?
- Why is it important to not call repository.Save if the employee is null?
The code is essentially a mystery to anyone that hasn't memorized the details of the Department.CreateEmployee method. Worse yet, I can make a lot of assumptions about this code - but every one of them is going to be wrong until I can prove that they are right by reading the code in Department.CreateEmployee.
So how do we refactor this code for better insight and more expressiveness? How do we change this code so that the assumptions a reader creates in their own mind are more likely to be correct - or better yet, are irrelevant because you don't need to even care about the details of the Department.CreateEmployee method? First and foremost, you need to understand what this code is supposed to be expressing - what it's supposed to be doing and why. If you don't know, you need to ask the person that wrote it. If they don't know (or aren't around), you need to find someone that can help you determine the intended behavior. Ultimately, you may need to dig into the Department.CreateEmployee method to figure it out for yourself.
In this situation, we were trying to express some business rules from a set of acceptance criteria on a user story:
- When creating an employee, the employee must belong to a department
- When creating an employee, require a first name, last name, and email address
This code is a factory method on the Department object that is used to ensure these rules are satisfied. In this case, if CreateEmployee returns a null, one of these rules was violated and we should not attempt to save the Department (with the associated employee, or lack there-of). If CreateEmployee returned an employee, we satisfied the rules and we can save the Department and it's associated employees.
The code that we have in place doesn't express any of that intent, though. What we really want is code that tells us that the employee creation failed - and probably why it failed. Fortunately, the changes can be extraordinarily simple.
public void SaveRequested()
{ EmploymentResult employmentResult = Department.Employ(firstName, lastName, email);
if (employmentResult.Success)
{ repository.Save(Department);
}
else
{ view.DisplayError(employmentResult.FailureReason);
}
}
UPDATE: Thanks to Martin Linkhorst for the comments, and making this code even more expressive (changing from Department.CreateEmployee to Department.Employ)
By creating an AddEmployeeResult object and supplying a few additional bits of information in it, our code became much more expressive of it's intent. We can now read the SaveRequested method and have a much better chance of knowing what it is really doing - try to create an employee and if it successfully created, then save it. Otherwise, display the failure reason.
What assumptions does your code create in the minds of the readers? Are these assumptions correct, incorrect, or unintelligible? And finally, what can you do to make your code assumption-free and/or assumption-irrelevant?