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 3 Results (finally)
Congratulations to Thomas Eyde who is the winner of Challenge 3! Thomas has won a copy of Design Patterns: Elements of Reusable Object-Oriented Software by Erich Gamma, Richard Helm, Ralph Johnson, John M. Vlissides along with a 3-user license pack for CodeIt.Once Refactoring. (The winner was randomly selected from all correct entries received.) Thomas Eyde was also quickest at the draw and had his correct submission in first...so he had his name thrown into the hat twice.

There were 10 correct entries out of a total of 10 submissions received before the deadline. From the feedback I received, it sounded like this challenge was the trickiest yet and started a lot of good discussion. I'll address all the points raised while going through the proposed solution. Correct entries were submitted by: Andrzej Kuczynski, Ginesh Kumar, Harry Chou, Jonathan Roe, Martin Clarke, Matt Payne, Navneet Gupta, Rasmus Faber, Rune Rystad and Thomas Eyde.

Discussion Points
  • Maintenance of Unit Tests: As most people noted, the unit tests did not need to be modified in any way throughout the entire refactoring. In fact, this is a common side-effect when performing refactorings. The primary benefit that unit tests bring while refactoring is giving you the confidence to make changes without being particularly worried about breaking existing functionality.  
  • Using Reflection to Set Class Members: A couple of people brought up the concern that using reflection within the unit tests to set private members of the ProjectActivity class may not be a good practice. (Implementation of this technique is found within the class Challenge3.Tests.Core.Domain.ProjectActivityReflectionUtil.) On the pros side, it allows you to keep code well encapsulated while still being able to unit test the functionality. But on the cons side, since the class member's are set using string literals to represent their names, you've lost compile time checking to easily adapt to changes. This leads to fragile unit tests...and if it's a pain to maintain unit tests, then developers will stop maintaining and running them altogether. As an additional drawback, it really breaks the rules of encapsulation altogether, treating private members as if they were public properties. Again, this leads to further degradation of unit test design. In hindsight, I believe it would have been better to expose the property setters, within ProjectActivity.cs, with a visibility of "internal," and then to grant the unit-test assembly explicit permission to access internal properties and methods of the Core package via the AssemblyInfo settings. So although I believe the reflection-setting technique may be appropriate in a few situations, it should probably be avoided as a rule of thumb.
Smell Refactorings

Smell: Conditional Complexity
Refactoring: Replace State-Altering Conditionals with State

As described in the challenge overview, there was only one smell to correct; albeit, it was a tricky one to resolve... (BTW, a good place to get an introduction to the state design pattern can be found at http://www.dofactory.com/Patterns/PatternState.aspx.)
  • Solution Class Diagram: The diagram below shows a summary of what an example, refactored class diagram looks like:

    State Class Diagram

    As shown above, the solution includes:
    • IActivityState: This interface allows various states to be used interchangeably by the ProjectActivity class. Some entries decided to make this an abstract class instead to encapsulate some duplicated code among the state classes. This was a fine approach as well. A couple others also integrated the template design pattern within the abstract base class to then call upon the state transition code within the concrete state classes. This was also a fine variation.
    • Concrete State Classes: The concrete state classes, ActivityNotStartedState, ActivityStartedState and ActivityFinishedState, carried out the actual transition details to move the ProjectActivity from one state to another. The greatest variation in submissions was how the state classes were given access to modifying the ProjectActivity members. I'll discuss this a bit more next.
  • Property Exposure: A major challenge in the completion of this exercise was in exposing the setters of the ProjectActivity properties without over-exposing them. The state engine needed to be able to modify these properties but publicly exposing the setters of these properties allowed them to be modified while bypassing the activity state engine. The drawback to this is that it becomes easy for someone working on the code to modify these properties directly; thus, possibly leading to an inconsistent or incorrect internal state. At the very least, the setters should only be exposed internally, as demonstrated by the following code submitted by Harry Chou and Matt Payne:

    Although internal accessibility allows other classes within the same assembly to directly modify the properties, it's reasonable to assume that developers working within the assembly should know the business rules concerning the use of the state engine. To further assist with encapsulation, the activity related classes may be segregated into their own assembly to ensure proper protection of internally visible properties and methods.

    Alternatively, Andrzej Kuczynski submitted an interesting approach; he modified the ProjectActivity class to be a partial class. Another extension of the partial class then included a nested, base, state class. In this way, the state engine could have direct access to the private members and avoid having to expose the setters at all. Bits of his approach follow:

    Generally, my preference is to eschew the use of partial classes. (I'm sure I'm going to step on some toes here...) In my opinion, partial classes are more difficult to easily comprehend, can lead to monolithic classes (even though the pieces are physically separated), and can result in unanticipated side effects when one partial class modifies the private members of another partial class. But, with that said, I felt Andzej's approach was a clever one to share and worth considering in your own work.

    Thomas Eyde also implemented a variation of this approach but did not join it directly to the state classes; instead, he provided a nested accessor class, within ProjectActivity, that exposed ProjectActivity's members to the state engine, but not to anything else - in my opinion, this was clean, provided good encapsulation and maintained a clean separation of ProjectActivity and the state engine.

  • Single Responsibility Principle: In Robert Martin's Agile Software Development, he describes a positive attribute of well-designed classes being that they have one clear responsibility. Because the solution to this challenge had to account for each type of transition uniquely, e.g. Not Started to Finished vs. Not Started to Started, it was difficult to avoid a few if/else statements in the state classes to dictate how the state-transitions should occur. (The if/else approach was what I ended up doing as well...but I didn't feel completely satisfied with this.) Arguably, this isn't a big problem as we're only talking about a few extra lines of code. But if the state-transition code were a bit more involved for each of the nine possible transitions, then each state class might start to get a bit messy as they attempt to account for each transition type. Jonathon Roe's submission included a separate class for executing each transition; e.g. NotStartedToStarted for instance. In this way, the transition classes could be used as different transition strategies. (Can you guess which pattern this is leading to?) Leveraging the strategy pattern inc conjunction with state may be overkill in simple cases, but worth considering as the transition logic becomes more complex.
I've attached a couple code submissions below to show how the refactorings were completed. Thanks to everyone who gave this one a try!

I hope you've enjoyed these challenges, that they've provided some value, and that my procrastination in getting the challenge 3 results out hasn't been too aggravating! The next challenge will involve using the Model-View-Presenter pattern to remove business logic from ASP.NET code-behind pages. And as soon I get out of my current iRobot Create experimenting phase, I'll have challenge 4 ready to go!

Billy

Posted 02-08-2007 12:04 AM by Billy McCafferty
Filed under: ,

[Advertisement]

Comments

Rob Eisenberg wrote re: Refactor It! Challenge 3 Results (finally)
on 02-08-2007 9:59 AM

Great post as always.  One quick note about http://www.dofactory.com.  I have noticed that their implementations of deisgn patterns are very un-C#-like.  They ignore major features of the lnguage in their implentations (notably, they make zero use of interfaces).  The implementations are very C++-like.  I have found Steven Metsker's writing (http://www.amazon.com/Design-Patterns-Steven-John-Metsker/dp/0321126971/sr=8-1/qid=1170946500/ref=pd_bbs_sr_1/105-3315155-0980402?ie=UTF8&s=books) to be an excelent book on C# design pattern implementaion for anyone who is interested in seeing a more C# side of things.  He also points out places where patterns are used within the .NET Framework, and places where perhaps they should have been used.

Billy McCafferty wrote re: Refactor It! Challenge 3 Results (finally)
on 02-08-2007 10:06 AM

Thanks for bringing that up Rob...online reference materials are certainly lacking when it comes to C# implementations of common design patterns.  I'd be interested in any websites people may have with respect to this.

Harry wrote re: Refactor It! Challenge 3 Results (finally)
on 02-12-2007 10:51 AM

Thank you, Billy for the great Challenge. I really enjoy all the challenges you provided. I am working on a new project now. I adopted things I learned from doing these challenges plus your ideas about MVP implementation. I really thank you for your help to the community.

I hesitate to give you more pressure, but I am looking forward to the next challenge and everything you write. (OK, I admit I don't have time to get into Robotics)

Gary wrote re: Refactor It! Challenge 3 Results (finally)
on 02-28-2007 9:26 AM

Hi Billy,

Your comment regarding reflection got me thinking about my own Unit Tests... I want to move to having factories for my objects

i.e. TransactionFactory.CreateCashBasedSwitch(<params>) and have the constructors as internal.  However, all my existing unit tests would break (yet still be valid) as they just build the objects using the ctors[ they'd still be valid 'cos the factory is a separate "unit" to the object it builds]

You have mentioned that there is a way to get round this problem via some setting in the AssemblyInfo.cs... could you give more details (please)?

Billy McCafferty wrote re: Refactor It! Challenge 3 Results (finally)
on 02-28-2007 2:07 PM

The modification includes InternalsVisibleToAttribute which is described on MSDN at http://msdn2.microsoft.com/en-us/library/system.runtime.compilerservices.internalsvisibletoattribute.aspx

Magnus Sälgö wrote re: Refactor It! Challenge 3 Results (finally)
on 04-16-2007 5:54 AM

http://www.dofactory.com has a payed version where you have a Net optimized version---

Sample:

.NET optimized sample code

The .NET optimized code demonstrates the same code as above but uses more modern, built-in .NET features. In this case the abstract class SortStrategy has been replaced by the interface ISortStrategy. The setter method SetStrategy has been implemented as a .NET property. The collection of students is implemented using a generic List, a new feature in .NET 2.0.

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)