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
Smell RefactoringsSmell: Conditional ComplexityRefactoring: Replace State-Altering Conditionals with State
- 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.
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:
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!
02-08-2007 12:04 AM