In my last entry, I outlined my ambitions for revisiting the code for my .NET 1.1 WinForms chess board control - specifically the rules engine. This component is responsible for analyzing each move made on the chess board control against the known rules of the game for:
- Legality - Is the piece being moved from/to legitimate squares on the board according to the type? Is the move blocked?
- Material Captures - Does the move result in material capture? This includes special capture moves such as en passant, which incidentally has a known bug.
- Check - Does the move result in either placing the player's or the opponent's king under direct attack?
- Check-mate - Does the move result in placing the player's or the opponent's king under attack that cannot be evaded?
There are also some additional methods in the engine that should probably be extracted, since they do not directly relate to the class’ core responsibilities, ie. Standard Algebraic Notation and FEN notations for recording moves and bit-board initialization and manipulation routines.
By the way, don't worry if you're not a chess fanatic or chess programming whiz - I certainly wasn't when I tackled the project! You should still be able to grok what’s going on – let me know if I’m glossing over material.
Before we get much further, you might want to pull down a copy of the original source code so you can follow along. Download it here. Opening the solution in Visual Studio, here’s where you’ll find the rules engine artifacts:
One thing to note here is the green orbs beside each element in the view – these are Subversion source control status flags that are provisioned via the Visual SVN plug-in for Visual Studio. Since we’re going to be ripping into the code, it is an absolute must that we use some form of code versioning. Subversion is free (as in beer), and is professional-grade, with features that I feel rival TFS. It has a small footprint and can be set up easily on a local box, web server or network share. In sum, it’s a great all-round tool for managing your code – I’ll do a posting on getting started with Subversion for .NET development in the near future for the curious.
The class file that contains the rules code I’ll be refactoring is contained in ChessRulesEngine.cs, a rollicking roller coaster of code over 1400+ lines deep (excluding comments) with 121 methods, 164 fields and eight types! Almost all of it necessary – I think. Well, except for the 24 #regions – which we all now know are completely evil (right, Bil?).
First thing we do: Kill All The #regions
I know that I promised to get into my strategies for tackling the high cyclomatic complexity, but before we can get to any refactoring, we need a clean surface. Looking at this old code, I cringe because it’s a little messy, and I want to make it more in-line with my current habits.
So I began by first removing all the #region statements so that I could see the code I was dealing with without having to mess around with little “+” swizzles and to reveal all the code in its naked glory. However, I did want to retain some markers about what each block of methods represented, so I did some global S&R to change all “#regions” to “// GROUP:” and “#endregions” to “// — GROUP END —” I then added GROUP as a task list macro (Tools—>Options—>Environment—>Task List) so that I could easily view and navigate to each section within the IDE:
Now, I could easily move around to each code grouping using descriptive markers. At this point, I committed my changes to the repository to serve as my new baseline. As an aside, when using any modern CVS, it is generally a good practice to provide a brief message about the changes you’re committing – getting into this habit will make your life a lot easier should you ever have to revert to a specific revision later on:
Next: In-depth code analysis
In my earlier post I mentioned two motivations that would provide an initial guide to establishing my refactoring plan: Maintainability and Cyclomatic Complexity. Each is complimentary in a yin/yang sense – by tackling methods and types with high CC scores, I would be making strides toward improving maintainability, and vice-versa.
Refactor! Pro provided me with a whack of methods with high CC scores. In general, methods that have a CC of 15 or higher are strong candidates for refactoring. CC metrics of 20–30+ should be aggressively refactored to split them up.
Now, while the Refactor! Pro metrics provide a good indication of some hotspots, I wanted to mine out some more information about my code to get a better sense of its overall health and to begin constructing a baseline “map” to evaluate my changes against. I needed to bring in a heavier duty tool.
Enter: NDepend
NDepend is a static (ie. non-runtime) code/assembly analysis tool for measuring an application's design (recall: all code is by nature, design) against a series of metrics that gauge type/assembly interdependencies, complexity, stability, couplings, abstraction and cohesion - among many others. Basically, all the things that help define code that not only functions well, but maintains well.
While initially a bit intimidating, NDepend can be your best friend once you get around the idiosyncrasies, and is invaluable in putting together a comprehensive picture of how an app is structured. Initially released as a free tool, it is now quasi-commercial at a cost of $400 USD for a single license. That said, you can use it for free (with some feature crippling) for non-commercial apps – works for me!
Running the analyses
There are some excellent tutorials on the NDepend site for getting started and how to use the tool, so I won’t go into a deep-dive in this post. Basically, we want to use the VisualNDepend GUI to create a new project from our VS2005 solution (sorry, no 2003 support) and focus on analyzing just the RNVirtualChessboard and RNChessBoardCommonTypes assemblies – we can remove the RNChessBoard assembly as that contains the code for the control GUI:
At this point, we can now launch the analysis and view the results in the VisualNDepend GUI (click to enlarge):
In the above capture I’ve reset the views in the GUI to focus just on code metrics. The middle pane with the bubble-blocks is a high-level view of the methods in the assemblies that are organized in relative size to one another according to the current metric, which is set to show cyclomatic complexity. The scores for CC are in turn provisioned via the CQL queries in the pane just below.
Right. CQL – That’s pretty cool. Wait, what?
CQL is an acronym for Content Query Language – in a nutshell, it allows you to run SQL-like queries against the metrics that NDepend has gathered so you can quickly view pertinent results according to adhoc criteria. OOTB, VisualNDepend has over 60 of these queries that you can run and modify. This feature alone makes NDepend a tool worth having, IMHO.
Note that we’re seeing only two methods, IsCastleLegit and DoPlayerMove, from the CC query as opposed to the 20–odd that Refactor! Pro was showing. This is because by default, the CQL query NDepend uses shows the top-10 results for methods with a CC score of 20 or higher. By editing the query to show methods with CC scores of >= 15, we get ten methods returned:
As cool as this is, we can glean even more targets through NDepend’s additional code quality queries, but for now I’d like to pursue the top-three in this list starting with DoPlayerMove, as I suspect that from here I will touch a lot of other methods. By right-clicking on the method in the results pane, I can access a context menu for drilling down into dependencies:
From here, I want to know what methods are directly consumed by DoPlayerMove – this query reveals the following shortlist:
Of these, the first two, UpdateMoveBitBoards and GetPieceTypeBitBoard have CC scores of 14 and 15. Almost immediately we can start to see the broad brushstrokes for our refactoring plan and we’re more aware of the dependencies that we can run up against.
Strategies for Dealing with Cyclomatic Complexity
Because cyclomatic complexity is a measure of the branching and looping elements that exist within a method or type, it makes sense that the strategies we’ll want to employ for refactoring will be concerned with mitigating the effects. Briefly, this means that we want to look at:
- Branching requirements
- Loop unrolling
- Switch/Case optimizations
- Segmenting code into smaller functions
Additionally, we want to be on the lookout for any low-hanging fruit that will improve the readibility of the code, such as introducing constants for any “magic numbers”, renaming methods where appropriate, collapsing redundant code, etc.
Refactoring-Ho!
For the remainder of the posts, we’ll be going through our target methods to begin sorting them out through the application of various refactorings. As an aide, I’ll be using the Refactor! Pro add-in to make suggestions on the changes we can make to improve the design of the code. While we should endeavour to recognize refactoring opportunities in our code without tools, they can provide us productivity gains and be instructive at the same time. However, as with any power tool, we need to employ a little TFD (thought-first development) and try to do as Norm Abrams recommends and measure twice, cut once.
Note: If you haven’t already a copy, I highly recommend getting Martin Fowler’s book, Refactoring: Improving the Design of Existing Code. While it is an older text (1st edition in 2000) it is the seminal work that defines the reasons and strategies for revising code through the identification and cataloging of “code smells”. Additionally, you may want to review Joshua Kerievsky's catalog of Refactoring to Patterns as well as Fowler’s online Refactorings Catalog which has current revisions and additions.
If we look at the DoPlayerMove code, we can readily see a number of “low-fruit” opportunities such as this block, which checks for the movement of either white rook as a pre-condition for determining the legality of a special defensive move called castling:
Refactor! Pro has picked up from my selection that I have the opportunity to employ several refactorings, including Replace with Constant – note that it even observed I already have a defined constant for the value that I can use! Taking a step back, we can also see that there’s an opportunity to employ the Extract Method refactoring:
However, now we’re starting to get ahead of ourselves: We need to backstop our code with some unit tests so that we can be assured that the changes we’re planning on making don’t break the build or introduce new bugs. For this, we’ll be using NUnit unit-testing framework v2.4.3 for .NET 2.0 and then adding a new project to our solution for containing our tests, which I’ll be covering in my next post!
In the next installment: Setting up Unit Tests
Your homework for the next installment is to install and become familiar with NUnit and a complimentary add-in for Visual Studio, TestDriven.NET as I will be covering the fundamentals of each only briefly as they relate to managing and running our tests. As always, if you have any questions feel free to post them below.