Tag Archives: code

K.I.S.S.

One of the talks I do explains how ‘simple’ in the K.I.S.S.1 principle is not the same as ‘easy‘. Sometimes the easy option can introduce complexities into the system that become detrimental and actually introduce complexity. The simplest example is testing. Not testing is the easy option; but it introduces uncertainty and complexity into your system that will be difficult to pin down at a later date.

That said easy and simple are not mutually exclusive, which is an important thing to remember, especially when you’re caught in the throws of trying to fix a supposedly simple problem.

I spent far too many hours yesterday trying to write an install script for one of our components. The underlying issue is simple: “As someone installing the component, I want the configuration files to reflect the idiosyncrasies of my setup so that I can compile the code”. Or, if you prefer BDD format: “Given I am installing the component, when I perform the compile step, then the compilation should work”.

It all boils down to a bindings file which specifies the location of a couple of libraries and includes. The defaults specified in that file don’t work on everyones system, although they do work on live2.

My salutation was to write some code that iterated through the files, check if any were missing, and if so, prompt the user to give the correct locations. Great, except the format of bindings.gyp is such that I needed to take a complex structure, flatten it, inject extra details so the user prompts made sense, and then reconstruct the complex structure from the flat one. Not wanting to hard code the format I then disappeared up my own backside with specially crafted config files and mappings from that to the format used by bindings.gyp.

nearly 200 lines of code in, deep in callback hell with grave concerns about whether my script would even pass code review I discovered some pretty nasty bugs that meant that minor configuration changes to a live server would cause an automated deploy to suddenly require user input, which we didn’t want. Adding logic to provide an override to this would make things even more complex and my nice, simple solution was disappearing out of reach.

It was then that it hit me that I was providing the wrong solution. This is something that really needs to be set once per environment and then left. It’s not going to be used a huge amount of time, it doesn’t need to be gold plated. With that in mind I wrote a simple shell script that checked for the presence of an environment variable. If the variable existed, use the bindings file it points to, if it doesn’t, use the default; simples.

All told, with error handling and everything, the script is 21 lines long. Not only that, but it provides a nice way to handle upgrades to the environment in live without having to redeploy.


1 Keep It Simple Stupid – in a nutshell, don’t overcomplicate things.

2 Something we wanted to maintain.

Refactoring as a Displacement Activity

Refactoring is good right? We constantly read about how we should be writing simple and easy code to do the minimum necessary and refactoring it as we go to improve it and expand what it can do. Unfortunately, this can lead to some really bad habits, especially if it’s used as a displacement activity.

What do I mean by displacement activity? Well lets consider a hideous project that is under specced, under resourced and overdue. Our poor old developer is demotivated and drowning under a sea of requirements that they don’t know how they’re going to implement, but what they have done is written the code that handles the command line arguments. This code works to spec, is even tested and could be considered complete. But, it’s taking user input. It’s not as neat as it could be. In fact it’s quite messy.

So what does our developer do? Do they tackle the next in the long list of requirements and try and get closer to completion, or do they refactor the working code to make it nicer? While we all know they should be doing the former, all to often it’s the latter that happens. Our developer goes off and refactors the working code to make it nicer.

Having seen this happen on a number of occasions it got me to thinking what was going on here. Why mess about with the stuff that works when there is a pile of stuff that doesn’t work to be getting on with. The answer is brutally simple, it’s a psychological trick.

In order to feel that they have had a productive day our developer needs to make progress. By tackling the difficult, as yet unwritten features, the developer is taking a risk. They may not be able to complete the task in the time they’ve allotted. They may run into problems. They may get stuck. They may fail. By refactoring the working code, on the other had, they’re tackling a problem that they know they can solve; after all, they’ve solved it already. What they’re doing now is a refinement.

Of course, once the refinement is completed our developer is still left with the mountain of requirements to complete that they didn’t want to tackle, and they now have less time to complete those requirements, but dealing with that problem has been shifted to something we can deal with tomorrow. In the mean time “progress”, for a given definition of progress, has been made because the existing code is now cleaner. A classic displacement activity.

Taken to its extreme this kind of thinking can lead to making nothing absolutely perfectly. You tend towards, but never actually achieve, the perfect solution for your problem. This is compounded by the fact that as soon as you start dealing with the edges of software development (input and output) things start getting messy. Perfection can’t actually be achieved and you’ll end up swapping one compromise for another ad-infinitum. Annoyingly we know all about this pitfall. the KISS principle and MVPs are all about avoiding this type of displacement and producing something that is important: working code. The irony is that this “perfect code” that we’ve wasted time crafting will no doubt get hacked about later as the system grows and the missing requirements are added. One day we may learn.

Why you should be documenting your code

It seems there are two schools of thought when it codes to documenting code. In one corner you’ve got those who feel that the “code should be expressive“; those that feel the “unit tests should document the code“; and there are those who argue that developers are lazy and the documentation gets out of date. These people will argue that there is no point documenting code. And in the other corner there’s me… well, me and the rest of my team, who have all seen sense after I’ve pointed out the error of their ways. 🙂

The rule for Javadoc in our team is “do it properly, or not at all“. I don’t mandate it because if you do you will just end up with rubbish. I do strongly encourage it though. The key word, however, is properly. I’m not just talking about ending the first sentence with a full stop, and making sure you have all the correct param, returns and throws tags; I mean well written, well thought out documentation.

I’ll handle the lazy developers argument first. The same argument can be made for not unit testing, and yet I doubt there are many people who would take the stance that not unit testing is OK. It’s part of development. Writing good Javadoc should also be a part of development.

But it’ll get out of date!“, I hear you cry. So not only has a developer done a half arsed job when updating some code by not updating the documentation, everyone who peer reviewed it missed that too. Please, feel free to hide behind that excuse. You may also want to stop doing reviews because you’re not doing them correctly. Also, what else are you missing?

The unit tests should document the code. Let’s consider a hypothetical situation. I’ve got your object and I’m calling a getter. I want to know if you’ll return null or not, because I may have some extra processing if to do if I do get a null value. You expect me to stop, mid flow, go find the unit tests, locate the relevant tests and work out if null is something that might come back? Really? If there are no tests dealing with null return values can I safely assume that null is never returned, or did you just forget to include that? I’ve never bought that argument.

As to the code being descriptive, well yes, it should. But it’s not always evident what’s going on. I can leap into our hypothetical get method and have a poke about to see what’s happening, but what if we’re getting the value from a delegate? What if that talks to a cache and a DAO? How far do I need to delve into your code to work out what’s going on?

And then we come on to the best argument of all: “You only need to document API’s“. It’s all API.

But what is good documentation? To help with that we’ll expand our scenario. Our getter has the following interface:

public String getName(int id);

What would most people put?

/**
 * Gets the name for the given id.
 * 
 * @param id the id
 *
 * @returns the name
 */
public String getName(int id);

It’s valid Javadoc, I’ll give you that, but it’s this kind of rubbish that has given the anti-documentation lot such great ammo. It’s told me nothing that the code hasn’t already told me. Worse still, it’s told me nothing I couldn’t have worked out from the autocomplete. I’d agree with you that having no Javadoc is better than that. This is why I say do it properly, or not at all.

The documentation needs to tell me things I can’t work out from the interface. The method takes an ID, you may want to be more explicit what the ID is. Are there lower or upper bounds for the ID? You return a String, are there any assumptions I can make about this String? Could the method return null?

/**
 * Returns the name of the user identified by the provided
 * user ID, or <code>null</code> if no user with the
 * specified ID exists. The user ID must be valid, that is
 * greater than 1.
 * 
 * @param id a valid user ID
 *
 * @returns the name associated with the given ID, or
 * <code>null</code> if no user exists
 *
 * @throws IllegalArgumentException if the user ID is less
 * than 1
 */
public String getName(int id);

This Javadoc does three things.

  1. It means I can easily get all the information I may need about the method from within the autocomplete dialog in my IDE, so I don’t need to leave my code.
  2. It defines a contract for the method so anyone implementing the code (or overriding it in the case of already implemented code) knows the basic assumptions that existing users already have.
  3. It provides me with a number of unit tests (or BDD scenarios) that I already know I have to write. While I may not be able to enforce the contract within the framework of the code (especially with interfaces), I can ensure the basic contract is followed by providing tests that check id is greater than 1 and that some non-existent ID returns null.

Writing good documentation forces you to think about your code. If you can’t describe what the method is doing succinctly then that in itself is a code smell. It forces you to consider the edge cases and to think about how people are going to use your code. When updating a method it forces you to consider your actions. If you’re changing our get method so that it will always return a default value rather than null will that affect other peoples logic and flow? What about the other way round when you’ve got a method that states it doesn’t return null – change that and you’re likely going to introduce NPEs in other code. Yes, the unit tests should pick that up, but lets work it out up front, before you’ve gone hacking about with the code.

Those of you familiar with BDD may spot that what you’re doing here is just performing deliberate self discovery with yourself. The conversation is between you, and you as a potential future developer. If, during that conversation, you can’t find anything to say that the interface doesn’t already tell you, then by all means leave out the Javadoc. To my mind that’s pretty much just the getters and setters on Javabeans where you know it’ll take and return anything. Everything else can and should benefit from good documentation.