Billy McCafferty : C#, Refactoring

IT Samurai School coming soon...

Billy McCafferty — Mon, 04 May 2009 02:32:00 GMT

As if my life wasn't boring enough trying to get S#arp Architecture 1.0 out the door...

Refactor It! Challenge 3 - The State of Things

Billy McCafferty — Tue, 19 Dec 2006 06:23:00 GMT

If you're not familiar with the Refactor It! challenges, please read this post. (Yes, this one is quite a bit overdue.) Now on to challenge 3...

Welcome back to another refactoring challenge! This challenge will pit your skills against the very useful State Design Pattern along with writing your own unit tests. It should take 1-2 hours for senior developers and a couple more for others. While the previous challenge made you focus only on the refactoring without worrying about writing unit tests, this challenge requires you to complete the refactoring along with writing your own unit tests. Here's the premise...

Background & Problem

This "refactoring to patterns" challenge surfaced during the past couple of weeks on an actual project. The real-world project is a project management tool that interfaces with a schedule management tool. The schedule management tool maintains a listing of project activities which include scheduled start/finish dates, actual start/finish dates (if they're available), and a remaining duration of the activity. (For the purposes of this challenge, many other activity properties have been dropped for simplification.) Since an API was not available to work with the scheduling tool's data, I had to communicate with its database directly. The problem was that quite a lot of business rules were executed when an activity changed from Not Started to Started, for example. ("Not Started" activities have scheduled start/finish dates but no actual dates; "Started" activities have an actual start date, a scheduled finish date but no other dates; and "Finished" activities have actual start/finish dates and no scheduled start/finish dates.) Taking a quick look at the class Challenge3.Domain.ProjectActivity shows how quickly this is getting out of hand.

Your Incentive

Up for grabs this week is the following book along with a CodeIt.Once Refactoring tool 3-User license pack! This seminal writing has become a required part of any serious developer's bookshelf.

Design Patterns: Elements of Reusable Object-Oriented Software
By Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides.

Instructions to Enter

Make sure you read all the instructions as they're different from the previous challenge!

Make sure you have NUnit installed; available at http://www.nunit.org/.
Download Challenge3.zip, found below.
Open the solution with VS 2005 and compile it.
Open NUnit, go to File/Open, then open <solution directory>/Challenge3.Tests/bin/Debug/Challenge3.Tests.dll. Click run and make sure all the tests pass. There should be a few unit tests that are ignored; but none should be failing.
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 with the refactorings, make sure all the unit tests are still passing and that no test is being ignored, zip up your solution and send to challenge3@emccafferty.com. As you may only win once, only submit a solution if you'd like to be eligible for the prize or let me know that you don't want to be entered for the drawing.
Because of the upcoming holiday schedule, the due date for your submission is Thursday night at midnight, January 4, US mountain time.

Challenge 3 Download Overview

The VS 2005 solution contains two projects:

Challenge3.Core: This class library contains the business logic layer of the application. This class library is further broken down as follows:
- The Domain folder contains all the entities and value objects. It also contains a sub-folder, ActivityState, which will hold the state design-pattern classes.
- The Utils folder contains a design-by-contract utility class written by Kevin McFarlane. The utility class is for all the Check.Requires you'll see throughout the code.
Challenge3.Tests: This class library contains the unit tests of the application. As there is no defined presentation layer, the unit tests serve as a portal to how the application works.

Challenge3.Domain.ProjectActivity: This domain class represents a specific activity within a project's schedule. In the real-world app that this was inspired by, I was communicating with a third party database and had to respect the rules of the data communications being performed by the third party client app. (I was bypassing the client app altogether with my own code but needed to ensure continued data integrity.) For example, if an activity is currently Not Started and becomes Started, then the scheduled start date needs to be set to a null date and the actual start date then needs to be set, accordingly. In addition to this, the remaining duration of the activity needs to be calculated and set. A simple implementation of the state pattern could remedy the conditional logic in the UpdateActivityStartAndFinishWith method. But here's the kicker...you need to take into account not only the object's current state but what state it will be in next. For instance, going from Not Started to Finished is not the same as going from Started to Finished. In the real-world app that this was inspired by, there were unique actions which needed to take place for each of the nine possible transitions; e.g. Not Started to Started, Not Started to Not Started (this isn't necessarily a state change, but the dates can still be modified), Not Started to Finished, Started to Not Started, etc. Your tasks are as follows:
1. Leverage the state design-pattern to simplify the change in states currently being handled by the method UpdateActivityStartAndFinishWith. Any state-related classes should be placed into the folder Challenge3.Domain.ActivityState. There are a few comments in the mentioned method...be sure to pay attention to them!
2. While implementing the state design pattern, existing unit tests should be maintained and new unit tests should be written for any new code. You shouldn't have to remove any unit tests from Challenge3.Tests.Core.Domain.ProjectProjectActivityStateChangeTests; but you may want to add a new suite of unit tests for the state classes which will be created. Currently 97% of Challenge3.Core.Domain is being unit tested. Your submitted solution must have at least 95% of Challenge3.Core.Domain still under unit tests. You can use a tool such as NCover or Clover.NET to verify unit test coverage. (Don't worry about unit testing the Utils namespace; consider this to be a third party DLL.)

Good luck and let me know if you have any questions. And remember, the first submitter with a correct entry gets his/her name thrown into the hat twice!

Happy holidays!

Billy

12/19/06 9:32 AM Mtn Time - Updated project to use US culture dates within unit tests.

Refactor It! Challenge 2 Results

Billy McCafferty — Fri, 01 Dec 2006 01:07:00 GMT

Congratulations to Michaël Willemot who is the winner of Challenge 2! Michaël has won a copy of Applying Domain-Driven Design and Patterns: With Examples in C# and .NET by Jimmy Nilsson along with a 3-user license pack for CodeIt.Once Refactoring. (The winner was randomly selected from all correct entries received.) Geert Baeyaert had the first correct submission and, incidentally, the first correct bonus entry as well.

There were 19 entries altogether with 17 correct entries. 11 entries attempted the bonus but only 5 entries correctly completed the bonus with a mock logger. Here's a run down of the correct entrants: Adam Vandenberg (bonus as well), Andrzej Kuczynski, Avi A, Bill Arnette, Brian Sherwin, Franck Tetard, Geert Baeyaert (first entry and bonus as well), Harry Chou (bonus as well), John Penokie, Jonathan Roe, Joshua McKinney (bonus as well), Matt Payne, Michael Willemot, Navneet Gupta, Rasmus Faber-Espensen, Rune Rystad (last week's winner, so not eligible for prize, but got bonus as well), Vitaly Davidovich. Nice work all!

Interesting Finds

Below are a few items of particular interest that were found in the submissions:

A few entrants included an abstract "join spec" that acted as the parent for the AndSpec, NotSpec and OrSpec. This helped centralize functionality and reduce duplicated code. Furthermore, a couple of entrants also made the spec pattern completely generic so its structure could be easily reused with different domain objects. Great call on both approaches.
I forgot to mention, and I'm glad I did, to remove the two unit tests that were affected when you removed the methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost. Quite a few people wondered whether they should be left alone, modified, or deleted. I'm glad it brought up this unintended conversation. Unit tests should be seen as a dynamic, living part of the code and are subject to the same level of refactoring as the "real" code. If a unit test is no longer pulling its weight or is no longer applicable, get rid of it. As Brian Sherwin noted, a great book to take a look at is Test-Driven Development by Kent Beck. This book demonstrates just how dynamic unit test development really is. With full-throttle TDD, the unit tests morph as much, if not more, as the production code itself.
The log4net bonus tripped up entrants mainly due to two reasons: 1) two separate logs needed to be generated, as was working in the original code, not just one, and 2) the singleton methods were tricky to mock. One solution to the first trip-point was to pass two loggers, one for the owner's log and one for the auditor's log to the nightly log process. The drawback to this approach is the maintenance headache with having to later on pass a third, forth, or nth logger to the class or to another class altogether. Alternatively, another approach is to supply a simplified Service Locator. This "logger locator" could be easily passed to anything and extended easily. I've attached a zip file, ExampleSolutions.zip, which contains four entries which used this type of approach (and contain a few variants of the spec pattern as well). The second trip-point was dealing with mocking the call to a method on a singleton. You can make a wrapper for the object, or, if you're in a real pickle, you can use a tool such as TypeMock which can mock just about anything, even sealed classes. The drawback to using a tool such as this is that it makes it easier to write more tightly coupled code. As mentioned in a previous post, Michael Feather's Working Effectively with Legacy Code has a number of great ideas for dealing with these types of situations without resorting to such a powerful tool. (You saw what the ring of power did to Frodo, didn't you?)
Someone brought up the question of what to do if you find yourself writing duplicated spec-creation code in different areas of the code. One solution is to create concrete "combination spec" classes for each of these variants. The problem with this is that it leads to the same type of combinatorial class explosion that we were running into before. Alternatively, for the duplicated spec combinations, you could create a CombinationSpecFactory class. This would have one or more creation methods that would return a Spec class; one method for each unique combination which is duplicated in more than one place. You might ask, what's better about having a factory with combination-spec-creation methods vs. having a number of concrete-combination spec classes? My preference of a factory over individual classes would be to keep the Spec class package clean; I want to keep my Spec class names to be as atomic as possible. I'd love to hear other thoughts on this issue!

Instead of describing the smell refactorings in detail, I'll let the code speak for itself. I've attached ExampleSolutions.zip below which contains four projects that have the bonus challenge included along with well-written log4net wrappers. Your comments are most welcome as to pros and cons of each approach.

I hope these refactoring challenges continue to be useful, offering plenty of food for thought. Stay tuned to the next challenge which will be rearing its ugly head sometime next week!

Billy

Refactor It! Challenge 2

Billy McCafferty — Thu, 23 Nov 2006 08:46:00 GMT

If you're not familiar with the Refactor It! challenges, please read this post. (Yes, this one is a little late in being published...but it's worth it!) Now on to challenge 2...

Welcome back to another refactoring challenge! I'm excited about this week's challenge and am sure it'll be interesting and challenging for all skill levels. It should take about 2 hours for senior developers and a couple more for intermediate developers. While last week's challenge served as a good introduction to refactoring in general, this week's challenge focuses on design pattern refactorings. Here's the premise...

Background & Problem

This application continues from the theme started in last week's challenge and includes a logging system for the local tiki bar. Each day, very early in the morning, a logging process kicks off which creates two logs summarizing orders placed on the previous day. An order has a customer, a date, a specific tiki drink, and a quantity of that drink. If the customer wants different types of tiki drinks, they need to place an order for each type. It's not your tiki bar, so that's just the way it is! The first log created each night is for the bar owner and includes all orders which had a total cost of at least $20 or had a quantity of at least 5. The second log is for the auditor and includes all tiki drink orders which had a total cost of at least $10 and were not for the drink called the "London Fog Cutter." (Don't ask me why the auditor doesn't want to know about these drinks...I just work here!)

The problem, in a nutshell, is two-fold. The first problem is that the filtering mechanism for figuring out which orders should be logged is getting quite complicated. The specification design pattern, a specialized version of the interpreter design pattern, needs to be employed to make the filtering easier to maintain. The second problem is that a bi-directional dependency exists between the data layer and the business logic layer. Consequently, it's very difficult to unit test the business logic layer without actually hitting the database. We'll need to introduce a separated interface, also known as the dependency inversion principle, to decouple the business logic layer from the data access layer. We'll then need to use a mock object so that we can predictably test the business logic layer without needing to worry about having a database around. (Tools exist to automate mock object creation, but let's we'll use one developed by hand.)

Your Incentive

Up for grabs this week is the following book along with a CodeIt.Once Refactoring tool 3-User license pack! This is a great book and will be a well-earned prize.

Applying Domain-Driven Design and Patterns: With Examples in C# and .NET
By Jimmy Nilsson.

Instructions to Enter

Make sure you read all the instructions as they're different from last week's challenge!

Make sure you have NUnit installed; available at http://www.nunit.org/.
Download Challenge2.zip found below.
Open the solution with VS 2005 and compile it.
Open NUnit, go to File/Open, then open <solution directory>/Challenge2.Tests/bin/Debug/Challenge2.Tests.dll. Click run and make sure all the tests pass. There should be a few unit tests that are ignored; but none should be failing.
After running the tests, confirm that two log were generated in <solution directory>/Challenge2.Tests/bin/Debug/. The log files are named Auditor.log and Owner.log and contain a number of order summaries pulled from the "database."
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 with the refactorings, make sure all the unit tests are still passing and that no test is being ignored, zip up your solution and send to challenge2@emccafferty.com. As you may only win once, only submit a solution if you'd like to be eligible for the prize or let me know that you don't want to be entered for the drawing.
Because of the lateness of this post and this week's holiday, the due date for your submission is Wednesday night at midnight, Nov. 29, US mountain time. It's not an easy one, so I wouldn't wait until Wednesday to get started!

Challenge 2 Download Overview

The VS 2005 solution contains two projects (at the moment):

Challenge2.Core: This class library contains the data access and business logic layers of the application. This class library is further broken down as follows:
- The Data folder contains all the data-access code. The only class within this folder, OrderDao, simulates talking to a real database by generating zero or more sample orders. The unpredictability of the results serves to simulate a real-life database.
- The Domain folder contains all the entities and value objects. It also contains a sub-folder, OrderFinder, albeit still in the same namespace, which holds the order-filtering logic.
- The Services folder contains the nightly logger process.
- The Utils folder contains a design-by-contract utility class written by Kevin McFarlane. The utility class is for all the Check.Requires you'll see throughout the code.
Challenge2.Core: This class library contains the unit tests of the application. As there is no defined presentation layer, the unit tests serve as a portal to how the application works.
Challenge2.Data: This isn't in the download, but you'll be adding this class library soon enough.

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. There may be other smells in the code, but these are the only ones that should be addressed.

Smell: Combinatorial Explosion
Refactoring: Replace Implicit Language with Interpreter

I have yet to write an application that doesn't include a bit of filtering capabilities. Furthermore, it's often the case that the filtering capabilities need to be extended throughout the life of the application. If only a couple of filters are required, then hard-coding the filter logic isn't that big of a deal. But if the number of filters grow or the logic becomes more complicated than just "AND"s, then using the interpreter design pattern may be an appropriate fit. But there's also the question as to whether filtering should be done by the database or by the code. As I prefer to keep database logic to the simplest CRUD (create/read/update/delete) functionality, I avoid putting complicated filtering logic into the database unless performance warrants otherwise. A good rule of thumb is that if you have a couple hundred items or less to filter, and you have more than a few filters that change regularly, then putting the filtering into the business logic layer is a good way to go; otherwise, the database should be doing the basic filtering. Alternatively, ORMs such as NHibernate provide their own implementation of the interpreter design pattern. So onto the smell...

Challenge2.Core.OrderFinder: This order-filtering class exposes the verbosely named methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost. Besides taking a while to read, these methods are difficult to extend, likely to change and are far from flexible. Without removing these methods (yet), refactor to the specification design pattern to make the filtering capabilities much more flexible. To assist, uncomment the unit tests within Challenge2.Tests.Core.Domain.OrderFinderTests one at a time, following the order suggested. Get each unit test working by implementing the implied code. Put all your "Spec" classes into the folder Challenge2.Core.Domain.OrderFinder but keep the namespace of those classes to be Challenge2.Core.Domain.
Challenge2.Core.Services.NightlyOrderLogger: This class creates writes two logs, one for the tiki bar owner and one for the auditor. To get the appropriate orders to log, it makes a call to the database to get all of yesterday's orders, and then leverages the OrderFinder class to filter, accordingly. Refactor this class to use the specification classes that you just created and delete the methods FindByNotTypeAndAtLeastTotalCost and FindAtLeastQuantityOrAtLeastTotalCost from Challenge2.Core.OrderFinder.

Smell: Dependency on Concrete Service
Refactorings: Extract Interface and Introduce Separated Interface

If you're wondering why you've never heard of the smell listed above, you probably won't be alone. (And if I ever write a book, I'll be sure to change that!) One of the most frequent issues I find with legacy code is direct dependencies on concrete classes that provide a service. A service, in this case, may be a data-access object which communicates with a database, an emailing utility class, an external web service, etc. A service is usually resource intensive, time consuming, or something that shouldn't be executed outside of a production environment. Having dependencies on concrete service classes does not present a problem if the code won't ever be unit tested, but neither will standing in the middle of a highway as long as there's no traffic...but I digress.

A solution for removing a dependency on a concrete service is to introduce, instead, a dependency on an interface. (I'll discuss more of the benefits of this in a moment.) But if the service class is still within the same physical assembly as the business logic layer, then there's nothing to stop a developer from bypassing the interface and going back to the concrete service class directly. To enforce a policy of services via interfaces, a separated interface may be introduced.

Challenge2.Core.Services.NightlyOrderLogger: Notice that this class creates a new instance of OrderDao to retrieve relevant orders from the database. This presents two problems. The first is that the process is time consuming from a unit test point of view. If you have a couple hundred unit tests and many of them require communications with a database, the unit testing process can begin to take 10 minutes or more. Once this happens, developers stop running unit tests altogether. (I experienced this on a previous project.) Furthermore, unit tests expect predictable results to verify code is working properly. Unless a database is reinitialized at the beginning of each test run, the results may vary and the tests will begin to break. This is another motivating factor for developers to stop running unit tests. (This was another unpleasant experience from a previous project.) One action you can take to mitigate this is to label any test that depends on a database with a "Database Test" categorization. With this in place, it's easy to "ignore" these tests within NUnit. But how can we test the business logic if we're ignoring much of it?

You can alleviate these problems by leveraging a mock object that simulates the service in an efficient and predictable manner. Take a look at Challenge2.Tests.MockObjects.Data.MockOrderDao. This class simulates the behavior of the real OrderDao class but returns the exact same results each time. Consequently, it's possible to use this mock object in place of OrderDao to verify that the business logic is working properly. Using mock objects in place of production data-access objects, you won't need to reinitialize the database on each test run. Carry out the following steps to introduce a Separated Interface and inject mock objects into your business logic layer for testing purposes.
1. Use the refactoring technique Extract Interface on the class OrderDao to create an interface called IOrderDao. Put the extracted interface into Challenges2.Core.DataInterfaces and have OrderDao implement it. (The interface should have just one method called GetOrderPlacedOn.)
2. Compile and run the unit tests to make sure everything is still working.
3. A mock object already exists, to simulate this data-access object, found at Challenge2.Tests.MockObjects.Data.MockOrderDao. Have this class also implement the IOrderDao interface.
4. Within Challenge2.Tests.Core.Services.NightlyOrderLoggerTests, there's a method called CanExecuteNightlyOrderLogger. Within this method, create an instance of MockOrderDao and pass it as a parameter to the method ExecuteNightlyLogging. You'll get a compilation error at this point. This is OK as we want the unit test to drive our code changes.
5. Add the following two unit test assertion to the same CanExecuteNightlyOrderLogger method: (You'll still be getting compilation errors from the previous step.)
```
Assert.AreEqual(3, summary.AuditorsLogEntryCount);
Assert.AreEqual(4, summary.OwnersLogEntryCount);
```
6. Modify the ExecuteNightlyLogging method of Challenge2.Core.Services.NightlyOrderLogger to accept an IOrderDao object. Be sure to use Check.Require to make sure it's not null. The name of the parameter should simply be orderDao. This will conflict with the new instantiation of OrderDao a few lines down. That's expected. Delete the line OrderDao orderDao = new OrderDao();. The NightlyOrderLogger is no longer dependent on a concrete service class; e.g. OrderDao. Instead, it only depends on the IOrderDao interface. But developers are still free to use OrderDao directly if they so wish. Now we'll introduce the separated interface.
7. Before proceeding, make sure everything compiles and the unit tests are passing.
8. Add a new project to the solution called Challenge2.Data. This project will contain classes which communicate with the database.
9. Move Challenge2.Core.Data.OrderDao to this new project and delete the Data folder from Chanllenge2.Core. (You'll need to add a reference to Challenge2.core from the Challenge2.Data project. While you're add it, also add a reference to Challenge2.Data from the Challenge2.Tests project.)
10. Finally, repair the namespace reference problems found within Challenge2.Core and Challenge2.Data, compile and confirm that all the unit tests are still passing.
As an optional, bonus step (and to get your name thrown into the hat twice), pass a mock log4net logger to NightlyOorderLogger in addition to the mocked OrderDao so that log files aren't necessarily generated during unit testing.

So what did we just accomplish? There are a few benefits to what we just did: 1) the unit tests are blazing fast as they don't depend on database communications, 2) the data-access and business logic layers are now loosely coupled, 3) the data-access layer can be easily mocked or even, albeit not likely, replaced with a completely different database communications layer, and 4) developers can no longer introduce new, bi-directional dependencies between the data-access and business logic layers. The last point goes a long way to keeping an application clean, testable and maintainable.

This refactoring challenge is quite a few steps beyond the simple, introductory refactorings we saw last week. But I hope I've illustrated that refactoring techniques can not only make a method cleaner, they can also provide direction for refactoring application architecture as a whole.

Good luck on the challenge and let me know if you have any questions!

Billy

Refactor It! Challenge 1 Results

Billy McCafferty — Mon, 20 Nov 2006 23:13:00 GMT

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:
/// <summary> /// Enumerate only <see cref="Customer"/>s who are of legal drinking age in the USA. /// </summary> private IEnumerable<Customer> USADrinkingAgeCustomers { get { foreach (Customer customer in this) if (customer.CanDrinkInUSA) yield return customer; } }
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

Refactor It! Challenge 1

Billy McCafferty — Tue, 14 Nov 2006 21:24:00 GMT

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 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
By Krzysztof Cwalina, Brad Abrams.

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

Refactor It! The Weekly Book Giveaway

Billy McCafferty — Tue, 14 Nov 2006 04:55:00 GMT

Each week, refactor my smelly project and win a free book
along with a CodeIt.Once Refactoring tool 3-User license pack!

Here's how it'll work...

Each week, beginning tomorrow, Nov. 13, a VS 2005 project will be provided which contains one or more specified "smells." The project may be a Windows App, a Console App, an ASP.NET Web App or even a simple Class Library; but it will always be in C# 2.0. Refactorings may range from simply introducing new methods to applying design patterns. The project will have one or more unit tests, verified with NUnit 2.0, to verify that the project is in working order.
You have until the specified time to submit your zipped, refactored solution. Instructions for submitting your solution will be presented when the contest begins. To be considered, the solution must be compilable with all unit tests passing.
The winner will be randomly selected from all "correct" submissions received. (I'll use my 2-year old to pick the name instead of Math.Random to make sure it's truly random!) As there are 1,000 ways to skin a cat - no offense to any PETA readers - "correct" entries will be subjectively determined by yours truly, Billy McCafferty. Along with an announcement of the week's winner, a results summary and refactoring analysis will be presented. "Correct" can generally be defined as the solution which presents simple, concise, and maintainable refactorings for the specified smells. Other "correctness" guidelines may be provided at the beginning of each challenge.
To help your chances, the first "correct" solution received will have two names thrown into the hat instead of one.
Only one entry may be submitted for each challenge and, unless specified at the opening of a contest, each participant may only win once.
The weekly contest will continue until I get refactoring-itus or I run out of books, whichever comes sooner.

Why refactoring...

Refactoring has been widely adopted as a vital technique for producing high quality software. It is important that we each embrace this practice into our development work. This contest will serve to present an overview of common smells found within code and discuss techniques to correct those smells for creating better software.

Resources to help...

The essentials are Refactoring by Martin Fowler and Refactoring to Patterns by Joshua Kerievsky. Other resources that may be helpful include Working Effectively with Legacy Code by Michael Feathers and Patterns of Enterprise Application Architecture by Martin Fowler.

Books up for grabs...

Design Patterns: Elements of Reusable Object-Oriented Software
By Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides.

Essential Windows Workflow Foundation
By Dharma Shukla, Bob Schmidt.

Framework Design Guidelines: Conventions, Idioms, and Patterns for Reusable .NET Libraries
By Krzysztof Cwalina, Brad Abrams.

Applying Domain-Driven Design and Patterns: With Examples in C# and .NET
By Jimmy Nilsson.

Essential ASP.NET With Examples in C#
By Fritz Onion.

Essential ASP.NET 2.0
By Fritz Onion, Keith Brown.

Stay tuned to devlicio.us and good luck!

Billy