Images in this post missing? We recently lost them in a site migration. We're working to restore these as you read this. Should you need an image in an emergency, please contact us at imagehelp@codebetter.com
Refactor It! Challenge 1 Results
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.
Smell Refactorings

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
  • 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:

    1. 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.
    2. Run the unit tests to make sure everything still works as expected.
    3. 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.)
    4. 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.
    5. Copy the code in Customer.IsEmailAddressValid to a new method within EmailAddress called 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.)
    6. Compile and verify that the CheckValidEmailAddress unit test is passing.
    7. Add another unit test called CheckInvalidEmailAddress, passing in an invalid email address this time, and assert that IsValid return false.
    8. Compile and verify that the CheckInvalidEmailAddress unit test is passing.
    9. Now, have Customer talk to the EmailAddress object directly to determine email address validity instead of the private method IsEmailAddressValid.
    10. Compile and run the unit tests again to make sure everything still works.
    11. 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: Magic Numbers
  • 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.
Smell: Long Method and Complicated Conditionals
  • 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 Customers 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!

Billy

Posted 11-20-2006 4:13 PM by Billy McCafferty
Filed under: , , ,

[Advertisement]

Comments

Justin-Josef Angel [MVP] wrote re: Refactor It! Challenge 1 Results
on 11-20-2006 7:08 PM

Great contest billy!

How about publishing the names of the people with a reasonable solution?

Not everyone has to be a winner but it'll be nice to get mentioned :)

Jeff Perrin wrote re: Refactor It! Challenge 1 Results
on 11-20-2006 8:40 PM

What would rock is if you could put all the submissions up for download (minus any names). It would be interesting to see how everyone refactored the code because, like you say, there's tons of "right" ways to do it.

Billy McCafferty wrote re: Refactor It! Challenge 1 Results
on 11-20-2006 9:38 PM

That's not a bad idea...I'll check with each submitter and post code, accordingly.  (I'll send you an email if you submitted code.)  It probably won't be until later this week, though, as I'm finishing up prep work for the next challenge.

Rasmus wrote re: Refactor It! Challenge 1 Results
on 11-21-2006 1:13 AM

Just a small comment:

You create a class to centralize the extraction of the config-file-settings. VS2005 actually has support for code-generating a type-safe configuration-file wrapper:

Select the project-properties, select the "settings"-pane, if necessary click "create settings file" and select the names, types, scopes and default-values. You can now access the setting through <namespace>.Properties.Settings.Default.<setting>.

Rob Eisenberg wrote re: Refactor It! Challenge 1 Results
on 11-21-2006 11:05 AM

While VS2005 does have support for "Settings," I have found the feature to be problematic.  The built-in Settings feature adds some additional gook to the config file and does some other VS Voodoo that I don't like.  Since its a trivial task to build this type of thing myself, I do, and enjoy the flexibility and peace of mind that comes with it.

About The CodeBetter.Com Blog Network
CodeBetter.Com FAQ

Our Mission

Advertisers should contact Brendan

Subscribe
Google Reader or Homepage

del.icio.us CodeBetter.com Latest Items
Add to My Yahoo!
Subscribe with Bloglines
Subscribe in NewsGator Online
Subscribe with myFeedster
Add to My AOL
Furl CodeBetter.com Latest Items
Subscribe in Rojo

Member Projects
DimeCasts.Net - Derik Whittaker

Friends of Devlicio.us
Red-Gate Tools For SQL and .NET

NDepend

SlickEdit
 
SmartInspect .NET Logging
NGEDIT: ViEmu and Codekana
LiteAccounting.Com
DevExpress
Fixx
NHibernate Profiler
Unfuddle
Balsamiq Mockups
Scrumy
JetBrains - ReSharper
Umbraco
NServiceBus
RavenDb
Web Sequence Diagrams
Ducksboard<-- NEW Friend!

 



Site Copyright © 2007 CodeBetter.Com
Content Copyright Individual Bloggers

 

Community Server (Commercial Edition)