For an overview of this contest, read this opening introduction. Now, onto this week's refactoring challenge...
As Fowler's subtitle describes, refactoring is about "improving the design of existing code"; an introduction to the subject can be found at refactoring.com. This first challenge includes a number of very common smells and straight-forward refactorings to remove those smells. For those of you who are new to refactoring, this particular challenge provides the perfect starting place. For those of who are refactoring guru's, I've got your can of worms coming soon!
Instructions to Enter
- Make sure you have NUnit installed; available at http://www.nunit.org.
- Download Challenge1.zip found below.
- Open the solution with VS 2005 and compile it.
- Open NUnit, go to File/Open, then open <solution directory>/Challenge1.Tests/bin/Debug/Challenge1.Tests.dll. Click run and make sure all the tests pass.
- Complete the refactorings as described below. Be sure to follow the style of the code that is already in place: classes, methods and public properties in PascalCase, private members in camelCase, etc. Following the style that's already there is always a good rule of thumb when modifying legacy code.
- When finished, make sure all the unit tests are still passing after you've completed the refactorings, zip up your solution and send to firstname.lastname@example.org. As you may only win once, only submit a solution if you'd like to be eligible for the prize.
Up for grabs this week is the following book along with a CodeIt.Once Refactoring
tool 3-User license pack!
Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries
By Krzysztof Cwalina, Brad Abrams.
Now Refactor It!
Below is a listing of smells that need to be fixed within the project. Each point indicates the file that contains the smell, a description of the problem and any guidelines for completing the refactoring. You may find quite a few other smells in the code as well, but only focus on the ones listed here.
Not all comments are bad. But many comments are an indication that something is too complex, that a business rule isn't being enforced, or that a method or class is doing too much in one place. Comments should describe why something is done and not necessarily what something does unless it's complex and there's no simpler way to do it. Another drawback to comments is that they need to be maintained along with the code. Instead, focus should be put toward making the code speak for itself; this gets rid of the comments and makes it more maintainable. The following are some comment smells found within the code that need to be fixed:
Smell: Magic Numbers
- Challenge1.Core.Domain.Customer: There are two comments concerning the allowable parameters. These comments don't seem to be enforcing much. Enforce these comments and get rid of them to make the code speak for itself. Don't worry about checking to see if the email address is valid. For a hint in the right direction, take a look at the constructor of Challenge1.Core.Domain.Order. (Guideline: Include the refactorings "Extract Method" and "Introduce Assertion.")
- Challenge1.Core.Services.EmailService: There's a comment on line 10. How can we get rid of this comment and make the "if" statement which follows a little more understandable? (Guideline: Include the refactorings "Extract Method," "Decompose Conditional" and "Introduce Assertion.")
A magic number is a constant which appears in the code itself. There are a few examples of magic numbers in the code that need to be fixed:
Smell: Long Method and Complicated Conditionals
- Challenge1.Core.Services.EmailService: There's a hard-coded email address and a hard-coded SMTP server within the method SendEmailTo. (Although not a number, this would still be classified as the Magic Number smell.) If the email address changes or the SMTP server changes, you'll have to make a modification and recompile the entire project. How should this be fixed to make it easier to change these items and to deploy it to various servers?
- Challenge1.Core.Domain.Customers: The same type of problem is found at the bottom of the method SendBeerSpamToCustomersOfDrinkingAge. Doh...who programmed this piece of ...!
- Challenge1.Core.Domain.Customers: This class contains the magic number "21." How should it be fixed? Is it appropriate to fix it in the same way as the EmailService fix, or do we really need to worry about changes to this number very often? Regardless, this is a magic number which isn't very explicit.
A long method is any method that has more than one responsibility or is too long to understand easily. As a rule of thumb, I try to make sure no method has more than about ten functional lines of code. In practice, I usually find that most of my methods are between three and ten lines long. (As a side note... Taking this to the extreme, is it incorrect to have a method with just one line of code? Not if it helps make your code more explicit and understandable.) In addition to improving readability, short methods make it much easier to spot duplicated code. For the challenge at hand, fix the following smell:
- Challenge1.Core.Domain.Customers: The method SendBeerSpamToAppropriateCustomers is an example of a long method. It has two obvious responsibilities, but it also has a couple of other, subtle responsibilities buried into the conditionals. The comment above the conditional could also be dropped if the conditional were made more explicit. Use the refactorings "Extract Method" and "Decompose Conditional" as many times as appropriate to improve readability and consolidate responsibilities. You needn't introduce any new classes to refactor this method.
To find help or explanations for a particular refactoring, "Extract Method" for example, try googling "extract method refactoring." Good luck and let me know if you have any questions!
11-14-2006 2:24 PM