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

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

  1. Make sure you have NUnit installed; available at http://www.nunit.org.
  2. Download Challenge1.zip found below.
  3. Open the solution with VS 2005 and compile it.
  4. 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.
  5. 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.
  6. When finished, make sure all the unit tests are still passing after you've completed the refactorings, zip up your solution and send to challenge1@emccafferty.com. 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

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.

Smell: Comments

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:

  • 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.")
Smell: Magic Numbers

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:
  • 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.
Smell: Long Method and Complicated Conditionals

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!

Billy

Posted 11-14-2006 2:24 PM by Billy McCafferty
Filed under: , , ,
Attachment: Challenge1.zip

[Advertisement]

Comments

RobC wrote re: Refactor It! Challenge 1
on 11-14-2006 6:33 PM

Is this this competition only open to US residents?  The tests fail on a computer with UK regional settings.  Does that count as a smell?!

Michal Grzegorzewski wrote re: Refactor It! Challenge 1
on 11-14-2006 6:40 PM

Please, correct: Krzysztof Carlina - > Krzysztof Cwalina (pronounced 'tsfalyna').

Billy McCafferty wrote re: Refactor It! Challenge 1
on 11-14-2006 6:55 PM

RobC,

Thanks for the heads up...I've modified the unit tests to work across different regions.  Please download the latest solution and let me know if it's still throwing red bars.  We wouldn't call that a smell...we'd call that a BUG! ;)  (And yes, this is open to anyone.)

mgrzeg,

Thanks for the correction...it has been done.

Peter wrote re: Refactor It! Challenge 1
on 11-14-2006 11:05 PM

If we get the correct solution and are chosen, can we choose which of the remaining books to receive as a prize?  I already own one of the books and am very interested in another.  

Also, will you give statistics each week of how many downloaded the project, how many submitted entries, and how many were correct?  I think that would be interesting.

Billy McCafferty wrote re: Refactor It! Challenge 1
on 11-14-2006 11:16 PM

Peter,

I'm trying to match the level of the book with the level of the refactoring challenge...but if you'd like to participate anyway, feel free to send in your solution and let me know that you don't want to be considered for the prize.  But in short, you'll have to wait for the challenge that's offering the book you're after.

As for the stats question, I should be able to provide most of those stats.

Peter wrote re: Refactor It! Challenge 1
on 11-15-2006 12:37 PM

I'm having a problem with NUnit and the app.config files.  I want to put some of the smelly magic numbers in an app.config file in Core.  The problem is that I can't get NUnit to read both config files:  the one from Core and the one from Tests.  I can get the Core tests to work, or the Configuration tests, but not both at the same time.  

Is it ok to copy the elements in the app.config from the Tests project (whatever) into the app.config file for the Core project?  Or copy the smelly elements of Core\App.config into Tests\App.config (probably a smarter solution)?  Or is there another way to work around this that I can't figure out because I'm not an NUnit expert?

Thanks,

Peter

Billy McCafferty wrote re: Refactor It! Challenge 1
on 11-15-2006 12:49 PM

Good question Peter.  Use Challenge1.Tests/App.config for storing any configuration needs.  By default, when Challenge1.Tests is compiled, it'll automatically copy/rename App.config into it's bin directory.  NUnit will then pick that config file up automatically.  So you don't have to worry about adding one to Challenge1.Core as well.  Essentially, Challenge1.Tests is your "presentation" layer, in a very loose interpretation of the word.

windywinter wrote re: Refactor It! Challenge 1
on 11-16-2006 6:24 AM

Billy,

Could you write something about approach to place enum inside class? From what you have written it seems that Enum class will serve as grouping construct for all kind of enums?

Regards,

v

Billy McCafferty wrote re: Refactor It! Challenge 1
on 11-16-2006 9:48 AM

windywinter,

I'd like to leave that one open as I don't want to recommend "the" best practice for organizing enum values.  Putting them all into a single class has worked well for me, but I'm interested to see how others organize theirs as well.

windywinter wrote re: Refactor It! Challenge 1
on 11-16-2006 1:50 PM

For me it is rather funny and ... interesting.

Dropping namesapces and nestig classes shows new possibilites! :P

for example static field on container class will be visible as global "variable" to inner structures :P

v

Gary wrote re: Refactor It! Challenge 1
on 11-16-2006 4:23 PM

Hi Billy,

In SendBeerSpamToCustomersOfDrinkingAge you appear to be sending spam on a per order basis i.e. if a Customer is of the lagal drinking age and they have 2 orders for homebrew in the last 6 months, then they will get 2 spam emails.  Is that what you meant?

Gary wrote re: Refactor It! Challenge 1
on 11-16-2006 4:27 PM

doh!!!

... just noticed the "break", ingnore my last post!

aq wrote re: Refactor It! Challenge 1
on 11-16-2006 6:56 PM

I found a small error in specification. "..of the method SendBeerSpamToAppropriateCustomers. Doh...who ..." should be "...of the method SendBeerSpamToCustomersOfDrinkingAge. Doh...who..."

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)