The last month has been something of a transitional time for me. I had been working out of my house for a handful of clients pretty much all summer, but now I’ve signed on for a longer term engagement out of state where I’m doing “craftsmanship coaching.” Basically, this involves the lesser-attended side of an agile transformation. There is no shortage of outfits that say, “hey, sign up with us, get a certification and learn how to have meetings differently,” but there does seem to be a shortage of outfits that say, “we’ll actually teach you how to write code in a way that makes delivering every couple of weeks more than a pipe dream.” I believe this state of affairs leads to what has been described as “flaccid scrum.” So my gig now is going to be working with a bunch of developers on things like writing modular code, dependency inversion, test driven development, etc.
This background is relevant for 2 reasons. First of all, it’s my excuse for why my posting cadence has dipped. Sorry ‘bout that. Secondly, it explains and segues into this post. What is software craftsmanship, anyway? I’m apparently teaching it, but I’m not really sure I can answer this question other than to say that I share a lot of opinions about what it means to write code effectively with people who identify this way. I think that TDD, factored methods, iterative, high-communication approaches, failing early, and testable code constitute are efficient approaches to writing software, and I’m happy to help people who want to improve at these things as best I can.
In that vein of thought, I’d like to offer some suggestions for tangible and easy-to-remember/easy-to-do things that you can do that are likely to improve your code. Personally, more than anything else, I think my programming was improved via random suggestions like this that were small by themselves, but in aggregate added up to a huge improvement. So, here is a series of things to tuck into your toolbelt as a programmer.
Make your variable names conversational
ComboBox cb = new ComboBox();
Ugh. The only thing worse than naming the variable after its type is then abbreviating that bad name. Assuming you’re not concerned with shaving a few bytes off your hard disk storage, this name signifies to maintainers, “I don’t really know what to call this because I haven’t given it any thought.”
ComboBox dayPicker = new ComboBox();
Better. Now when this thing is referenced elsewhere, I’ll know that it probably contains days of some sort or another. They may be calendar days or days of the week, but at least I know that it’s talking about days, which is more than “cb” told me. But what about this?
ComboBox mondayThroughFridayPicker = new ComboBox();
Any doubt in your mind as to what’s in this combo box? Yeah, me neither. And that’s pretty handy when you’re reading code, especially if you’re in some code-behind or any kind of MVC model-binding scheme. And, of the objections you might have, modern IDE’s cover a lot of them. What if you later want to add Saturday and Sunday and the name becomes out of date? Easy to change now that just about all major IDEs have “rename all” support at your fingertips. Isn’t the name a little over-descriptive? Sure, but who cares — it’s not like you need to conserve valuable bytes of disk space. But with cb name, you know it’s a combo box! Your IDE should give you that information easily and quickly and, if it doesn’t, get a plugin that tells you (at least for a statically typed language).
Try to avoid booleans as method parameters
This might seem a little weird at first, but, on the whole your code will tend to be more readable and expressive if you don’t do this. The reason for this is that boolean parameters are rarely data. Rather, they’re generally control parameters. Consider this method signature:
void LogOutputToFile(string output, bool useConsole = false)
This is a reasonably readable method signature and what you can infer from it is that the method is going to log output to a file. Well, unless you pass it “true”, in which case it will log to the console. And this tends to run afoul of the Single Responsibility Principle. This method is really two different methods kind of bolted together and its up to a caller to figure that out. I mean, you can probably tell exactly what this method looks like:
void LogOutputToFile(bool useConsole = false)
{
if(useConsole)
Console.WriteLine(output);
else
_someFileHandle.WriteLine(output);
}
This method has two very distinct reasons to change: if you want to change the scheme for console logging and if you want to change the scheme for file logging. You’ve also established a design anti-pattern here, which is that you’re going to need to update this method (and possibly callers) every time a new logging strategy is needed.
Are there exceptions to this? Sure, obviously. But my goal here isn’t to convince you never to use a boolean parameter. I’m just trying to get you to think twice or three times about doing so. It’s a code smell.
If you type // stop and extract a method
How many times do you see something like this:
_customerOrder.IsValid = true;
_logfile.WriteLine("Finished processing customer order");
//Now, save the customer order
if(_repository != null)
{
try
{
_repository.Add(_customerOrder);
}
catch
{
_logfile.WriteLine("Oops!");
_showErrorStateToUser = true;
_errorMessage = "Oops!";
}
}
Would it kill you to do this:
_customerOrder.IsValid = true;
_logfile.WriteLine("Finished processing customer order");
SaveCustomerOrder();
and put the rest in its own method? Now you’ve got smaller, more factored, and descriptive methods, and you don’t need the comment. As a rule of thumb, if you find yourself creating “comment bookmarks” in your method like a table of contents with chapters, the method is too big and should be factored. And what better way to divide things up than to stop typing a comment and instead add a method with a descriptive name? So, when you find you’ve typed that “//”, hit backspace twice, type the comment without spaces, and then slap a parenthesis on it and, viola, new method signature you can add.
Make variable name length vary with scope size
This seems like an odd thing to think about, but it will lead to clearer code that’s easier to read. Consider the following:
Globals.NonThreadSafeSumOfMostRecentlyProcessedCustomerCharges = 0;
for(int i = 0; i < _processedCustomers.Count(); i++)
Globals.NonThreadSafeSumOfMostRecentlyProcessedCustomerCharges += _processedCustomers[i].TotalCharge;
Notice that there are three scopes in question: method level scope (i), class level scope (_processedCustomers) and global scope (that gigantic public static property). The method level scope variable, i, has a really tiny name. And, why not? It's repeated 4 times in 2 lines, but it's only in scope for 2 lines. Giving it a long name would clog up those two lines with redundancy, and it wouldn't really add anything. I mean, it's not hard to keep track of, since it goes out of scope one line after being defined.
The class level scope variable has a more descriptive name because there's a pretty good chance that its declaration will be off of your screen when you are using it. The extra context helps. But there's no need to go nuts, especially if you're following the Single Responsibility Principle, because the class will probably be cohesive. For instance, if the class is called CustomerProcessor, it won't be too hard to figure out what a variable named "_processedCustomers" is for. If you have some kind of meandering, 2000 line legacy class that contains 40 fields, you might want to make your class level fields more descriptive.
The globally scoped variable is gigantic. The reason for this is twofold. First and most obviously, it's in scope from absolutely anywhere with a reference to its containing assembly, so it better be very descriptive for context. And secondly, global state is icky, so it's good to give it a name that discourages people from using it as much as possible.
In general, the broader the usage scope for a variable/property, the more context you'll want to bake into its name.
Try to conform to the Principle of Least Surprise
This last one is rather subjective, but it's good practice to consider. The Principle of Least Surprise says that you should aim to minimize the learning curve or inscrutability of code that you write, bearing in mind a target audience of your fellow developers (probably your team, unless you're writing a more public API). As a slight caveat to this, I'd say it's fair to assume a reasonable level of language proficiency -- it might not make sense to write horribly non-idiomatic code when your team is likely to become more proficient later. But the point remains -- it's best to avoid doing weird or "clever" things.
Imagine stumbling across this bad boy that compares two integers... sort of:
public bool Compare(int x, int y)
{
Console.WriteLine("You shouldn't see this in production.");
_customerCount = x;
return x.ToString() == y.ToString();
}
What pops into your head? Something along the lines of, "why is that line about production in there?" Or maybe, "what does a comparison function set some count equal to one of the parameters?" Or is it, "why compare two ints by converting them to strings?" All of those are perfectly valid questions because all of those things violate the Principle of Least Surprise. They're surprising, and if you ask the original author about them, they'll probably be some weird, "clever" solution to a problem that came up somewhere at some point. "Oh, that line about production is to remind me to go back and change that method. And, I set customer count equal to x because the only time this is used it's to compare customer count to something and I'm caching it for later and saving a database write."
One might say the best way to avoid this is to take a break and revisit your code as if you're someone else, but that's pretty hard to do and I would argue that it's an acquired skill. Instead, I'd suggest playing a game where you pretend you're about to show this code to someone and make mental note of what you start preparing yourself to explain. "Oh, yeah, I had to add 39 parameters to that method -- it's an interesting story, actually..." If you find yourself preparing to explain something, it probably violates the Principle of Least Surprise. So, rather than surprising someone, maybe you should reconsider the code.
...
Anyway, that's all for the tips. Feel free to chime in if you have any you'd like to share. I'd be interested to hear them, and this list was certainly not intended to be exhaustive -- just quick tips.
