Don’t do this


public Configuration loadConfiguration(String fileName){
if(configuration == null){
configuration = parseFile(fileName);
}
return configuration;
}


It’s really not a very good thing to do.

When I read this code, I had an Aha! moment. There were portions of “Aha! So this is what has made my life misery for the past several hours.” but mostly it was “Aha! There’s a general principle at play which this violated.”

For the non-programmers reading, here is the problem with this code. Other code which uses this function will look like this:

//blah, blah, blah
...
myConfig = loadConfiguration(myConfigFile);
...


To a programmer reading this, the meaning of the code seems obvious. What this code does is initialize some configuration from the specified file. Except… It doesn’t.

What the code really does, is initialize some configuration from the specified file… the first time it is called. The next time, and the next time, and the next time, it returns the same result, which it has remembered from the first run through.

Remembering results isn’t a bad thing, per se. It’s called caching and it’s a fundamental programming technique. It’s not really the evil thing that the code does. It’s not hard to rewrite this code, so that while still caching, it’s not evil.


public Configuration loadConfiguration(String fileName){
//if we're being asked for a different file than the last time, reload
if(configuration == null or fileName != oldFileName){
configuration = parseFile(fileName);
}
oldFileName = fileName;
return configuration;
}


The evil thing is that most of the time, the original code ignores the name of the file it was given. From the position of someone using this code, asking for configuration from myFile and receiving configuration from yourFile instead is not a good thing.
If the code didn’t take in a fileName, it would probably be okay. The function

public Configuration loadConfiguration();

is pretty clear; it loads the current configuration. It doesn’t allow you to say, “Load this file” but at least it doesn’t lie to you. Ignoring parameters is lying and it’s not okay.

(Ok, this probably isn’t making for glamorous reading – but I need to document this so I don’t forget it.)