Arnon Rotem-Gal-Oz

Dr. Dobb's Bloggers

Code Readability: Documentation vs. Refactoring

June 18, 2008

Technical documentation of the code/project seems like a worthy goal. After all, if we aren't living in a vacuum someone will need to understand our code/design and maintain it, use it to develop new stuffm, etc.
On the other hand writing documentation can get tedious, boring, hard to maintain, and whatnot -- so you run across all sorts of ways to deal with it.

Technical documentation of the code/project seems like a worthy goal. After all, if we aren't living in a vacuum someone will need to understand our code/design and maintain it, use it to develop new stuffm, etc.

On the other hand writing documentation can get tedious, boring, hard to maintain, and whatnot -- so you run across all sorts of ways to deal with it.

Consider, for example, the following:

On a recent article in IBM's DeveloperWorks Paul Duvall suggests you use automation to generate various documentation artifacts like UML diagrams based on the source code (using ANT tasks and UMLGraph), ERDs (with SchemaSpy) etc.

What I think is that while both of these efforts can help satisfy a customer-specific requirement for "comprehansive documentation"* they have very little value in making anyone understand anything about your code. UML diagrams can only help if they are created at a higher level of abstraction than the code (which means they'd be hand-crafted) and if GhostDoc can understand your code enough to create anything useful, it means that your method and parameter names are self-descriptive anyway.

In a previous post I mentioned that I prefer to rely on tests, short methods and meaningful names for readability. I'll talk about tests in another post. For this installment let's look at the other two. I think it would be better demonstrated by an example.

This class needs a lot of explanations if you want to understand what exactly going on here. So you can set yourself up to writing a lot of comments and trying to figure things out -- or assuming this class was fully tested (which it wasn't, but that's another story) try to refactor it until we get something meaningful (I will omit the added tests though for brevity).

Step 1: First If

It seems that when we have a first frame we want to keep it aside, so let's extract method all the if code to EnqueueFrame

if (Frame != null) EnqueueFrame(Frame);

Okay, so now we look at EnqueueFrame. The code we see here talks with the m_WaitingFrame private member (which is a Dictionary of <Guid, WaitingFrame>();. The first thing we'll do is to rename it to FramesQueue. Now the more interesting thing is that the code here has to do with managing this FramesQueue and isn't directly related to the containing class.

We can either subclass the Dictionary class or we can add an extention method to Dictionary<Guid,WaitingFrame> to handle this for us. We'll do that, and then refactor the If again:

The advantage of what we've achieved thus far is both better OO design (separation of concerns) and enhanced readability by using intention revealing names and notation.

Step 2: The foreach loop

Again we'll start with Extract Method, we can now remove the definition of FreeProcessNum from the beginging and we get:

var FreeProcessNum = GetFreeProcessNum(ref FreeProcessId);

but this code is not really clear. For one thing, we have to rename FreeProcessNum to FreeProcessesCount to make it more legible. and we have an ugly and hard to follow ref variable. It is probably better to apply the Single Responsibility principle and seperate this into two distinct methods, so we'd get:

Thank you MS for adding Linq and Lambda expressions :). The same can be done for GetNextFreeId

Step 3: The Two Ifs and the Rest

Taking a deep look at the code we can see that the rest of the method tries to find a free processor and if there are enough processors send the frame, otherwise it should send the top prioritized. We can also spot a bug here that two different threads can get the same Processor and then try to send a message to it one after the other. Another potential bug comes from the way the maximal priority is found. There's an assumption there that the max priority would be 1000. While it isn't likely to happen it is still a hard coded assumption.

Anyway, if we continue and apply the same principles that got us here (Single Responsibility Principle, Don't Repeat Yourself, Intention revealing methods, coherence and opening classes to add specific functionaliy) we get the original method to look like the following:

You should note that this is not the end of the refactoring (e.g. we should still handle the WaitingFrame, FrameQueue and the ProcessesList which we are called here) we just took a look at a single method.

While there might still be a need for an occasional explanatory remark , this little exercise demonstrates that we can gain a lot in the way of clarity by refacroting code and keeping up a few simple principles. Oh yeas... and what we got at the end of the process is not just readable code, but also a more maintainable, better designed code that can move forward and evolve further as the system changes.

* I don't underestimate the value of generating full documentation when there's such a requirement from a customer. I would prefer to convince a customer that having such a Write-Only document is a complete waste of time and trees but sometimes you can't help it. Generating documents in these situations can be a life-saver.

Dr. Dobb's encourages readers to engage in spirited, healthy debate, including taking us to task.
However, Dr. Dobb's moderates all comments posted to our site, and reserves the right to modify or remove any content that it determines to be derogatory, offensive, inflammatory, vulgar, irrelevant/off-topic, racist or obvious marketing or spam. Dr. Dobb's further reserves the right to disable the profile of any commenter participating in said activities.

Video

This month's Dr. Dobb's Journal

This month,
Dr. Dobb's Journal is devoted to mobile programming. We introduce you to Apple's new Swift programming language, discuss the perils of being the third-most-popular mobile platform, revisit SQLite on Android
, and much more!