This article speaks briefly about refactoring and code structure, focusing on several stylistic thoughts that have crept up time and time again throughout my career as a software developer. The art of software development has many facets, however many of the goals that distinguish great software from its counterparts are often ignored, even more often ignored unintentionally. The quality of a piece of software lies in many more areas than its ability to fulfil a requirement or perform a task. The development lifespan of most software spans far beyond the initial release, and often beyond the initial developer. Sooner or later, someone else is not only going to have to read and understand your code, but also extend and enhance it in meaningful ways. This someone may very well be you, but after several months have passed by it doesn’t really matter whether its a new face or the initial developer. If the code cannot be understood and meaningfully digested then performing maintenance upon it and extending it usefully will come at a far higher cost than is necessary. This leads to one of the additional goals of good software: a transparent, intuitive, and consistent structure that can be readily understood by someone other than the initial developer at a later date.
First, let me define in my own way what I interpret the qualities of transparent, intuitive, and consistent structure to mean. From this you should be able to translate the meanings into your own understandings and gain meaning from the conclusions I draw and the ideas that I’m trying to present.
Transparecy
When I refer to code that is transparent I imply that its structure, style, and solution do not “hide” the thought processes behind that solution from any would-be reader. Transparent code should go out of its way to present the ideas behind the solution without introducing any “magic” or “voodoo” that occurs behind the scenes. Sometimes, the nature of a solution involves the use of such magic, often the most graceful solutions “appear” to have virtually no code at all. However, as the developer of the code you completely understand at the time of writing “why” the solution is working and should, therefore, be able to provide sufficient transparency within that code as to explain its purpose and behaviour. If a third-party process or disconnected workflow is accomplishing part of a task for your code (for example, the use of an HttpHandler to solve certain parts of a global web application problem) then add clear and concise documentation to the code about how that part of the solution is working. Don’t leave it hidden behind the curtain. Expose the fact that some other component is solving part of the problem at hand. There is no need for brevity in comments and code-documentation. Large comments do not increase the size of a compiled assembly and they are not included in the delivered solution. Last time I checked, no-one was too worried about an additional 8K of comment text in a source code file stored in a development environment or source control solution either. However, documentation is not the only facet of transparency. The structure of any software solution often has an almost tastable opacity to it. You can open up a code file in an editor and almost instantly smell and taste whether this code will lend itself towards easy reading (if you’re lucky), or a large heirogliphic decryption exercise (as with most projects).
Intuitive Design and Consistent Structure
These two facets of good software go hand-in-hand together. Defining something as being intuitive is very difficult. What is intuitive to one person may be foreign and misleading to another. I have often presented solutions that I find to be incredibly intuitive and natural only to find that one of my respected peers has developed an equally valid and elegant solution in a completely different direction. However, this disparity can easily be resolved through consistent structure. Although the thinking behind a particular solution may be foreign and far from intuitive to me at first, I can learn about the approach and begin to develop an intuition for it as long as the structure of the overal solution is consistent. In essence, you are training my mind to your way of thinking and with every method it becomes intuitive due to its consistent nature. The solution itself becomes predictable, despite the lack of early intuition. Here then, consistency is absolutely key to that training. Switching tracks often in the solution, ignoring consistency, will interfere with this learning process and the chances are that the solution will never make intuitive sense.
What does this have to do with rocket ships?
Part of where this is leading is to discuss coding style. I’m not taking about the age-old (and quite futile) discussions of whether curly braces live on the same line as a declaration, or whether variables need to be prefixed with a certain convention of coded letters. That was covered in the section on intuition and consistency. (It doesn’t matter how variables are named or where the brace lives as long as the style is consistent; it becomes intuitive quickly). However, there are aspects of style that do make a big difference to the readability and “maintenance cost” of your code. It is in this that rocket ships come into play, in my first example of coding clearly. How many times have you encountered a code block that looks like this?
if (ctl != null) {
if (ctl.Text != String.Empty) {
if (shortName != “”) {
fullName = shortName + ctl.Text;
}
else {
string err = “Short name was empty.”;
log.Error(err);
throw new Exception(err);
}
}
else {
string err = “The control text was empty.”;
log.Warn(err);
}
}
else {
string err = “The control was null.”;
log.Error(err);
throw new Exception(err);
}
It doesn’t take long to figure out what this code is doing. However, it can take a while longer to figure out what this code is meaning. I can imply from the first construct that we don’t want to try to access the Text property of the variable ctl if it is null. That’s a good safety check and it’s fairly clear from the first if statement. However, it is not so clear what the rules around ctl.Text and shortName being empty actually are. Is it invalid to change the variable fullName if both ctl.Text and shortName are empty. That seems to be the case here, but it isn’t very clear and maintenance or bug-fixing on this code might encounter some ambiguity as a result. Not only that, but we’ve already wasted valuable minutes guessing at the actual intent of the code. Furthermore, ctl.Text is tested against String.Empty whereas shortName is tested against “”. Was this intentional? Can ctl.Text and shortName be null, but not empty? This is also fairly unclear. In fact, the only line of code that performs any real work is nestled away in one of the longest barrels of a rocket ship. It takes a few moments to isolate the fact that the point of this code is to set a variable called fullName to the combination of shortName and ctl.Text. This is the effect of “hiding” the solution I was discussing in transparency. What if the code was structured like this instead?
// Assert that the reference to the suffix TextBox control is valid
if (txtSuffix == null) {
string err = “The reference to the suffix TextBox was null. The value of the control cannot be retrieved and the full name will not be updated as a result.”;
log.Error(err);
throw new Exception(err);
}
// Assert that a suffix has been provided (this is not an error, a short name alone will suffice)
if (Strings.IsNullOrEmpty(txtSuffix.Text)) {
string err = “The suffix TextBox does not contain a value. This is not an error, but no suffix is available to append to the full name.”;
log.Warn(err);
}
// Assert that a short name has been provided (this is an error, a suffix alone is not allowed)
if (Strings.IsNullOrEmpty(shortName)) {
string err = “A short name was not provided. A full name cannot consist of a suffix alone, the operation will be aborted.”;
log.Error(err);
throw new Exception(err);
}
// Update the full name
fullName = shortName + txtSuffix.Text;
Here, the rather ambiguous ctl has been renamed to txtSuffix. Again, the naming convention is immaterial as long as the pattern is consistent throughout the project. However, it is always useful to name variables in a way that presents a little more data than the misleading ctl. More importantly, the code has been refactored into a series of assumptions rather than a single nested contortion. The same logical checks are still being performed, but the rocket-ship-shaped (they always remind me of the side-scrolling video game Defender; like rocket-ships pointing from left to right) gourdian knot of if statements has been disbanded. By separating out the thoughts into individual blocks of code, the assertions that the developer is trying to make are presented to the reader much more clearly. It is useful for me to think of writing code in the same manner as writing project documentation or an article for a magazine, trying at all times to make it readable, engaging, clear, and succinct. A comment can be added to each of the assertions to explain the reason for its presence. While the nested if statements could have been commented, the refactored structure of the code allows for more meaningful placement of these comments as each assertion cleanly stands alone. It is no longer unclear in which order the events will occur or what the statechart for this code would look like. It’s not that such information couldn’t be derived from the nested if statements, but when you charge by the hour for your work, every minute is valuable. The error information being written to logs or reported in exceptions has been expanded to give a more verbose explanation for the decisions that the code is making. This not only aids in debugging and system maintenance, but also makes the code itself inherently more readable.
This is but the simplest of examples where a minor restructure effort and a little thought to naming and logging has made a section of code vastly more readable and transparent. I know from personal experience (as I’m sure you all have come across too) that there are far deeper nested if statements lurking throughout code you will be one day asked to maintain. Perhaps you wrote it, or perhaps you’re the lucky one that just gets to fix it when it breaks. Either way, it serves as a great reminder to pay heed to code quality when you are debugging a problem in production on a Friday night at 8pm with little end in sight and you isolate the problem down to a one-thousand line, twelve level-deep nested gourdian knot that just threatened to take away your weekend. It’s very easy when coding to think that because it’s intuitive right now, that your code will always make instant sense to you. That almost certainly won’t be the case once several months have passed by and it certainly won’t be the case when someone other than you needs to understand and maintain that code. After writing a method or structuring a class, take a second to think just how transparent and consistent the code really is. Does it fit with the convention used in other parts of the project? Are there any “magic” methods or tricks that make something work that won’t be quite as intuitive in a few weeks time? Do the log messages (<rhetoric>you do use log messages, right?</rhetoric>) give ample meaning to the actions of the code? Most importantly, can the code be refactored now that the final solution has been hashed out? Nearly every method I write can be refactored in some way or another once I’m finished. I would never deliver a speech without reading, editing, and refactoring several times. Code shouldn’t be any different.
But…good code documents itself!
Rubbish! I’ve heard this statement countless times in the context of similar discussions. I argue instead that familiar code documents itself, and often only for the period of time for which you’re working on it. I know parts of the Linux kernel that are absolutely outstanding pieces of code. They have been refactored and architected many times over by some of the smartest developers on the planet. If you stripped out all of the comments and presented just the raw lines of code, would that really be self-documenting? Would the deepest parts of the mutex queue management library be instantly clear to all who gazed upon them simply because the code is so amazingly awesome? Doubtful! It is true that a consistent and transparent code structure can remove the need for a lot of excess documentation, however it does not mean that helping someone along with a few insightful comments is simply a waste of keystrokes. In any solution worth charging for, the design and final implementation of each method will have been thought about many times. There should not be any code that simply exists because it has always been there, and fairly early on in the development life-cycle a good body of code will be developed that remains essentially unchanged throughout the lifetime of the project. It makes sense that methods with such longevity that have been so carefully crafted and positioned just so within your code are therefore worthy of a few lines of documentation and logging to affirm their presence. With a combination of forethought, refactoring, consistent day-one logging, and an investment in comments and documentation, there is no reason that transitioning code from one developer to another should incur any unnecessary cost. Given that most code will be refactored or maintained at some point in its life, I can guarantee that the time investment up front is worth it. QA and support will adore the additional logging, the customer will appreciate the ability to release patches and updates on schedule, and any future developer suckered with having to work with your code in the future will be thankful that at least one other person out there took the time to make their day a little easier. The global quality of software increases or decreases with the next method you write. Make it count.
This one makes the “Stuart Thompson’s Top Ten Peeves” list in code reviews.
Programming
Assertions, Coding, Programming
