More Readable Code – Conditionals

I read an article by William Durand about Object Calisthenics (worth a read) that gives an overview of this unique software development practice.

These rules focus on maintainability, readability, testability, and comprehensibility of your code. If you already write code that is maintainable, readable, testable, and comprehensible, then these rules will help you write code that is more maintainable, more readable, more testable, and more comprehensible.

Here are the 9 rules for Object Calisthenics (links will take you to the article):

1. Only One Level Of Indentation Per Method
2. Don’t Use The ELSE Keyword
3. Wrap All Primitives And Strings
4. First Class Collections
5. One Dot Per Line
6. Don’t Abbreviate
7. Keep All Entities Small
8. No Classes With More Than Two Instance Variables
9. No Getters/Setters/Properties

The one that I would like to focus on is the rule “Don’t Use the ELSE Keyword”.

An easy way to remove the else keyword is to rely on the early return solution… The condition can be either optimistic, meaning you have conditions for your error cases and the rest of your method follows the default scenario, or you can adopt a defensive approach (somehow related to Defensive Programming), meaning you put the default scenario into a condition, and if it is not satisfied, then you return an error status. This is better as it prevents potential issues you didn’t think about.

So the basic idea is:

1. No else statements
2. Use early return for successful conditions
3. If process reaches the end of the method (or condition code block) return an error state.

I would like to review a couple of conditional code blocks I have come across and how I would refactor those.

Problem #1

//Problem

foreach(var item in items)
{
	if(item.IsState == false)
	{
		continue;
	}
		
	if(item.value.IsAnotherState == false)
	{
		continue;
	}
	
	item.Disable():
}

In this code block we have two if-statements inside a loop. The confusing thing is that by using the continue statement it is more confusing as to what is the “success” condition. You might think that the idem.Disable() is the failed condition but it isn’t. That is the success condition. So what we have here is an optimistic condition where the continue statement is called for non-successful states.

To make it more readable and maintainable, I would refactor this to something like:

//Solution

foreach(var item in items)
{
	if(!item.IsState && !item.value.IsAnotherState)
	{
		item.Disable():
	}
}

Here the success condition is that condition which calls the item.Disable(). Any other condition or state will skip this action.

Problem #2

//Problem

foreach (var item in items)
{
	if (!item.IsState)
	{
		item.DoSomething();
	}
	else
	{
		item.DoSomethingElse();
	}
}

Here we have an else statement that we want to get rid of. So under Object Calisthenics we would rewrite this as:

//Solution

foreach (var item in items)
{
	if (!item.IsState)
	{
		item.DoSomething();
        continue;
	}

    item.DoSomethingElse();
}

This actually fits with the no else statement rule, but still looks weird to me, and weird code is less maintainable code.

Maybe we can make this less weird by encapsulating the condition in a method.

//Solution

foreach (var item in items)
{
	TryDoSomethingOrElse(item)
}

private void TryDoSomethingOrElse(var item)
{
    if (!item.IsState)
	{
		item.DoSomething();
        return;
	}

    item.DoSomethingElse();
}

Maybe the return is more “known” and less weird looking, but still kinda looks not-quite-right to me.

So what looks the most right to me?

The original problem where we have both an if- and an else-statement looks the most right to me… haha!

Guess I’m not ready for the heavy calisthenics.