This is the second post in a short series of post about developper advice ([1], [3], [4]). The first one listed 16 pieces of advices in the form of headlines. This post briefly explains the first 4:
0000: No bool to bool conversion
I dont know how many times I've seen code that basically converts from one boolean value to another. Like the following (real life, in-production, but obfuscated to protect the innocent) Java script code:
function IsEditorReady()
{
if (editorReady)
{
return true;
}
else
{
return false;
}
}
Why, oh why?
A variation of this is to have some boolean expression as the if-clause. A further variation is to have a series of nested ifs. Or to assign the boolean value to a variable instead of returning it. But still it's just converting something thats already a boolean to ... a boolean. And it's harder to read.
0001: Trust the implementation
When running on top of a big standard library take advantage of that. Don't re-implement the standard library functionality. Don't, for instance, write your very own sting tokenizer because you "want to be sure it works". Firstly you can be pretty sure your version doesn't work in all the corner cases. Secondly its a waste of time. Thirdly it's harder to read, than code using the standard library implementations. So c'mon; take advantage of those huge standard libraries we most often have today.
Writing code is error-prone. Period. But writing imperative code is much more error-prone than writing declarative code. Why? Because imperative code tries to tell the machine what to do step by step (and there are lots and lots of steps), while declarative code tells the machine what the intent is, and leaves the step by step details to the underlying implementation (which we trust, remember?).
Just as importantly declarative code is easier to read, for the exact same reason: It expresses intent, rather the method of reaching the intended goal. Consider this piece C++ of code:
{
alarmMutex.lock();
alarmList.push_back(alarm);
alarmMutex.unlock();
}
And then the alternative:
void XYZClient::pushAlarm(const alarm_log &alarm)
{
boost::mutex::scoped_lock lock(alarmMutex);
alarmList.push_back(alarm);
}
The second simply declares that the alarmMutex should be held until the end of the current scope. The first one tries to say when the mutex should be taken, and when it should be released ... and fails at doing so correctly. Granted it's just a matter of adding a try catch ... or is it...what about cases where the call to unlock fails?...oh, the headache.
If you open up your doors, vampires will enter and drink your life blood...or in terms of OO software: If a class does not protect it private parts properly some other code will grip them, squeeze them and feast on their last bit of blood...well maybe that wasn't really in terms of software either, but consider this code:
public class Test : AbstractTestElementComposite
{
private readonly IDictionary
new Dictionary<string, string>();
// snip
public IDictionary
{
get { return headerProperties; }
}
// snip
}
The problem is that the internal headerProperties dictionary is fully exposed by the getter. Oh, there's no setter, but that's just false protection, since vampirish code like this is allowed:
class Vampire
{
void SuckBlood(Test t)
{
t.HeaderProperties.Clear();
t.HeaderProperties.Add(“Alive?”, “Don’t know”);
}
}
This shows that the headerProperties dictionary isn't really kept private in this example. For a thourough write-up on the vampire issue I recommend Kevlin Henneys Encapsulation and Vampires article at StickyMinds.
That it for now, more advice follows.