Congratulations to Rune Rystad who is the winner of Challenge 1
! Rune has won a copy of Framework Design Guidelines
by Krzysztof Cwalina and Brad Abrams along with a 3-user license pack for CodeIt.Once Refactoring
. (The winner was randomly selected from all correct entries received.) For the record, Jeff Perrin had the first correct submission...so he got his name thrown into the hat twice and gets a nice pat on the back. ;)
Wow, what a response! There were 21 correct entries out of a total of 25 submissions received before the Sunday night deadline. Due to the number of entrants, if I haven't responded with feedback, you can assume your submission was correct. I was glad to see that people enjoyed working through the challenge and hope you all got something out of it.
Before digging into the results and the solution itself, I wanted to bring up a couple of notes about refactoring and the challenge itself. There's an important aspect of refactoring that the challenge doesn't illustrate very well: a tenant of refactoring is keeping the code in a working state as much as possible and to only have one unit test failing at a time. Consequently, refactoring should be seen as a process, and not just a before and after state. I'll give an example of this when discussing the example solution. On another note, a couple of entrants brought up the point that there were other smells in the code not mentioned in the challenge description...heck yeah there were! If you've worked with legacy code, and we all have, you'll see smells all over the place. At times, the smells are so bad that it's difficult to make any forward progress with refactoring without feeling that you have to gut the whole thing. In times like these, Michael Feather's Working Effectively with Legacy Code
can be a great resource. These comments have inspired me to include one such example in the the next challenge.Interesting Finds
Below are a few items of particular interest that were found in the submissions:
- Some created a new class called
EmailAddress to handle checking the validity of an email address. Although not required for this particular challenge, doing so consolidates the business logic, makes it more reusable and treats an email address as a first class citizen; and it usually should be.
- To fix the "magic number" smell in the Customers class, Adam Vandenberg included a clever usage of iterators to get the list of all customers of drinking age:
- Some created a configuration-settings, reader class that centralized the mining of the config file into a single class. This is an example of what Martin Fowler calls a Gateway. I thought it was a great idea and have included it in the example solution. (Another good time to use a gateway is when working with data stored in Session.) The major benefit of using this approach is that if the config (or session) keys change, then you only need to change them in one spot outside of the config file.
Now let's take a look at one
possible solution to the refactorings. There are many different approaches to the challenge, and many of them are equally valid.Smell: Comments
Smell: Magic Numbers
- Challenge1.Core.Domain.Customer: There were two comments at the top of this class dictating business rules for "age" and "emailAddress." As mentioned before, some entries included an
EmailAddress class to encapsulate the logic for verifying the validity of an email address. This particular refactoring, unsurprisingly enough, is called Move Method. Detailing how this is done, without breaking existing code, you would take the following refactoring steps:
- Introduce a new method in the
Customer class called
IsEmailAddressValid. This method would accept an email address and return whether or not it's valid. This refactoring is called Extract Method. This method could then be used to enforce a valid email address in the constructor of
Customer. Now we can get rid of the comment concerning passing a valid email. But should the
Customer class really know how to check for the validity of an email address? We'll address this concern after we make sure we haven't broken anything.
- Run the unit tests to make sure everything still works as expected.
- Create a unit test class, called
EmailAddressTests for a yet, non-existent
EmailAddress class. (This class is in the solution as Challenge1.Tests.Core.Domain.EmailAddressTests.)
- Add a unit test called
CheckValidEmailAddress which creates an instance of an
EmailAddress, passing in a valid email address, as a string, to its constructor. In the unit test, assert that the
IsValid method of the object returns true for a valid email address. (You're correct if you're asking yourself, what IsValid method? This is called test driven development...we'll let the unit test tell us what code to write.
- Copy the code in
Customer.IsEmailAddressValid to a new method within
IsValid. Don't remove the duplicate code from
Customer yet...we want to fix the currently breaking unit test before moving on to anything else. (Incidentally, some entries checked for the email address, validity check in the constructor of
EmailAddress while others left it to other classes to ask
EmailAddress if it was valid. Either way may be correct depending on the business requirements and both approaches were considered correct for this challenge.)
- Compile and verify that the
CheckValidEmailAddress unit test is passing.
- Add another unit test called
CheckInvalidEmailAddress, passing in an invalid email address this time, and assert that
IsValid return false.
- Compile and verify that the
CheckInvalidEmailAddress unit test is passing.
- Now, have
Customer talk to the
EmailAddress object directly to determine email address validity instead of the private method
- Compile and run the unit tests again to make sure everything still works.
- Delete the
IsEmailAddressValid method from the
Customer class and run the tests again.
The preceding steps included the refactorings Introduce Assertion, Extract Method, Move Method, and Decompose Conditional. No wonder it took so many steps! Although the details of additional refactoring will not be broken down to such detail, I wanted to show how a refactoring is carried out at a lower level. (And if I haven't promoted it enough already, I highly recommend reading Martin Fowler's Refactoring to see each of these refactorings examined in more depth.) The above seems quite tedious, but the entire cycle takes just a couple of minutes and gives you confidently, working code (aka - peace of mind).
If you've looked at the solution code, you may have noticed calls to
Check.Require to implement the Introduce Assertion refactoring. This code calls a wonderful design-by-contract class originally written by Kevin McFarlane. You can read more about it, and design-by-contract in general, in a previous post. Although it seems overkill and unnecessary, sprinkling
Check.Require all over your code will save you hours upon hours in the debugger. (Just give it a whirl for a couple of days and you'll be convinced.)
- Challenge1.Core.Services.EmailService: If I write anything about the comment-smell in this class, I'd create a "duplicated blog content" smell. I.e. see the details of the first point for all the nitty-gritty details.
Smell: Long Method and Complicated Conditionals
- Challenge1.Core.Services.EmailService: There was a hard-coded SMTP server name and email address in the code. The correct solution was to put these values in App.config so that they could easily be changed between development and production environments. Taking it a step further, the example solution includes a new class, Challenge1.Core.ConfigSettings, which provides centralized access to the config settings. A couple of entries suggested this approach.
- Challenge1.Core.Domain.Customers: This class also had a hard-coded email address; the previous point's fix also applies to this one. Additionally, this class had a hard-coded value of "21" to represent the legal drinking age in the United States. The refactoring Replace Magic Number with Symbolic Constant has been used in the example solution to give the number more meaning. I chose to put this magic number into a constant instead of the config file because it is highly unlikely, in spite of how much we tried before we were 21, that this age will change. But the constant's name serves to make the code more readable. Finally, this class had a couple of strings, representing the spam's subject and body, which never changed. Although not implicated before, I've also placed these strings into constants as well. In a real-world situation, these probably would be pulled from a database or flat-file...neither of which I want to get into now! If requirements dictated that either of these be changed in the future, that would be the time to refactor the code to a more maintainable solution.
- Challenge1.Core.Domain.Customers: The method
SendBeerSpamToCustomersOfDrinkingAge has been sliced and diced, using the refactorings Extract Method, Decompose Conditional, and Replace Magic Number with Symbolic Constant to make it, arguably, easier to understand. The method name
ConditionallyTrackAndSendSpamTo may seem long, but it fully describes that the method sends spam, conditionally, and tracks any emails that are sent. You should never shorten a method name with acronyms or leave out a responsibility from the name. If you follow this rule of thumb, then long method names may be a sign that a method is trying to do too much. Perhaps we should Extract Method again to enable us to make the method name shorter and more concise?
In carrying out the refactoring Decompose Conditional, for this class, I introduced the method
IsHomebrewOrderPurchasedInPastSixMonths to the order class. Now there's a method name! This is an example of a decomposed conditional taken to the extreme. It perfectly summarizes the conditional in a readable fashion; but it's easy to see that this could get out of hand if we need to add other methods such as
IsMoonshineOrderPurchasedInPastEightMonths. We could parameterize both the type and the timeframe, but what if we add another variable to the mix...er...mess? If that never happens, then this method may be adequate, but if that very probable situation does occur, it may be time to refactor to the interpreter design pattern. But again, if there's never a need to expand the filtering capabilities for orders, then we needn't invest the time, nor added complexity, of introducing this design pattern. (Down pattern-itus, down! Good dog.)
As I've mentioned, there are many, many
ways to refactor this solution and what I've described is just one of them...with that, I open the floodgates to constructive criticism! ;) To make a preemptive strike, there could be a few more unit tests and the
class is taking on a few too many responsibilities; but since I've spent way too much time on this post already, I'll leave the remaining smells as food for thought!
Stay tuned for tomorrow's challenge!
11-20-2006 4:13 PM