How do I edit a chain of if-else if statements to adhere to Uncle Bob's Clean Code principles?

Question

I'm trying to follow Uncle Bob's clean code suggestions and specifically to keep methods short.

I find myself unable to shorten this logic though:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

I cannot remove the elses and thus separate the whole thing into smaller bits, cause the "else" in the "else if" helps performance - evaluating those conditions is expensive and if I can avoid evaluating the conditions below, cause one of the first ones is true, I want to avoid them.

Even semantically speaking, evaluating the next condition if the previous was met does not make sense from the business point of view.

---

edit: This question was identified as a possible duplicate of Elegant ways to handle if(if else) else.

I believe this is a different question (you can see that also by comparing answers of those questions).

• My question is checking for the first accepting condition to end quickly.
• The linked question is trying to have all conditions to be accepting in order to do something. (better seen in this answer to that question: https://softwareengineering.stackexchange.com/a/122625/96955)

Answer 1

Ideally I think you should extract your logic for getting the alert code/number into its own method. So your existing code is reduced all the way down to

{
    addAlert(GetConditionCode());
}

and you have GetConditionCode() encapsulate the logic for checking conditions. Maybe also better to use an Enum than a magic number.

private AlertCode GetConditionCode() {
    if (CheckCondition1()) return AlertCode.OnFire;
    if (CheckCondition2()) return AlertCode.PlagueOfBees;
    if (CheckCondition3()) return AlertCode.Godzilla;
    if (CheckCondition4()) return AlertCode.ZombieSharkNado;
    return AlertCode.None;
}

Answer 2

The important measurement is complexity of the code, not absolute size. Assuming that the different conditions are really just single function calls, just like the actions are not more complex than what you've shown, I'd say there's nothing wrong with the code. It is already as simple as it can be.

Any attempt to further "simplify" will indeed complicate things.

Of course, you can replace the else keyword with a return as others have suggested, but that's just a matter of style, not a change in complexity whatsoever.

---

Aside:

My general advice would be, never to get religious about any rule for clean code: Most of the coding advice you see on the internet is good if its applied in a fitting context, but radically applying that same advice everywhere may win you an entry in the IOCCC. The trick is always to strike a balance that allows human beings to easily reason about your code.

Use too big methods, and you are screwed. Use too small functions, and you are screwed. Avoid ternary expressions, and you are screwed. Use ternary expressions everywhere, and you are screwed. Realize that there are places that call for one-line functions, and places that call for 50-line functions (yes, they exist!). Realize that there are places that call for if() statements, and that there are places that call for the ?: operator. Use the full arsenal that's at your disposal, and try to always use the most fitting tool you can find. And remember, don't get religious even about this advice as well.

Answer 3

It's controversial whether this is 'better' than the plain if..else for any given case. But if you want to try something else this is a common way of doing it.

Put your conditions in objects and put those objects in a list

foreach(var condition in Conditions.OrderBy(i=>i.OrderToRunIn))
{
    if(condition.EvaluatesToTrue())
    {
        addAlert(condition.Alert);
        break;
    }
}

If multiple actions are required on condition you can do some crazy recursion

void RunConditionalAction(ConditionalActionSet conditions)
{
    foreach(var condition in conditions.OrderBy(i=>i.OrderToRunIn))
    {
        if(condition.EvaluatesToTrue())
        {
            RunConditionalAction(condition);
            break;
        }
    }
}

Obviously yes. This only works if you have a pattern to your logic. If you try to make a super generic recursive conditional action then the setup for the object will be as complicated as the original if statement. You will be inventing your own new language/framework.

But your example does have a pattern

A common use case for this pattern would be validation. Instead of :

bool IsValid()
{
    if(condition1 == false)
    {
        throw new ValidationException("condition1 is wrong!");
    }
    elseif(condition2 == false)
    {
    ....

}

Becomes

[MustHaveCondition1]
[MustHaveCondition2]
public myObject()
{
    [MustMatchRegExCondition("xyz")]
    public string myProperty {get;set;}
    public bool IsValid()
    {
        conditions = getConditionsFromReflection()
        //loop through conditions
    }
}

Answer 4

Consider using return; after one condition has succeeded, it saves you all the elses. You might even be able to return addAlert(1) directly if that method has a return value.

Answer 5

I've seen constructions like this considered cleaner sometimes:

switch(true) {
    case cond1(): 
        statement1; break;
    case cond2():
        statement2; break;
    case cond3():
        statement3; break;
    // .. etc
}

Ternary with right spacing can also be a neat alternative:

cond1() ? statement1 :
cond2() ? statement2 :
cond3() ? statement3 : (null);

I guess you could also try to create an array with pair containing condition and function and iterate over it until first condition is met - which as I see would be equal to Ewan's first answer.

Answer 6

As a variant of @Ewan's answer you could create a chain (instead of a "flat list") of conditions like this:

abstract class Condition {
  private static final  Condition LAST = new Condition(){
     public void alertOrPropagate(DisplayInterface display){
        // do nothing;
     }
  }
  private Condition next = Last;

  public Condition setNext(Condition next){
    this.next = next;
    return this; // fluent API
  }

  public void alertOrPropagate(DisplayInterface display){
     if(isConditionMeet()){
         display.alert(getMessage());
     } else {
       next.alertOrPropagate(display);
     }
  }
  protected abstract boolean isConditionMeet();
  protected abstract String getMessage();  
}

---

This way you can apply your conditions in a defined order and the infrastructure (the abstract class shown) skips the remaining checks after the first has been meet.

This is where it is superior over the "flat list" approach where you have to implement the "skipping" in the loop that applies the conditions.

You simply set up the condition chain:

Condition c1 = new Condition1().setNext(
  new Condition2().setNext(
   new Condition3()
 )
);

And start evaluation with a simple call:

c1.alertOrPropagate(display);

Answer 7

First of all, the original code isn't terrible IMO. It's pretty understandable and there's nothing inherently bad in it.

Then if you dislike it, building up on @Ewan's idea to use a list but removing his somewhat unnatural foreach break pattern:

public class conditions
{
    private List<Condition> cList;
    private int position;

    public Condition Head
    {
        get { return cList[position];}
    }

    public bool Next()
    {
        return (position++ < cList.Count);
    }
}


while not conditions.head.check() {
  conditions.next()
}
conditions.head.alert()

Now adapt this in your language of choice, make each element of the list an object, a tuple, whatever, and you're good.

EDIT: looks like it isn't as clear I thought, so let me explain further. conditions is an ordered list of some sort; head is the current element being investigated - at the beginning it is the first element of the list, and each time next() is called it becomes the following one; check() and alert() are the checkConditionX() and addAlert(X) from the OP.

Answer 8

Supposing all functions are implemented in the same component, you could make the functions retain some state in order to get rid of the multiple branches in the flow.

EG: checkCondition1() would become evaluateCondition1(), on which it would check if previous condition were met; if so, then it caches some value to be retrieved by getConditionNumber().

checkCondition2() would become evaluateCondition2(), on which it would check if the previous conditions were met. If previous condition was not met, then it checks for condition scenario 2, caching a value to be retrieved by getConditionNumber(). And so on.

clearConditions();
evaluateCondition1();
evaluateCondition2();
evaluateCondition3();
evaluateCondition4();
if (anyCondition()) { addAlert(getConditionNumber()); }

EDIT:

Here's how the check for expensive conditions would need to be implemented in order to this approach work.

bool evaluateCondition34() {
    if (!anyCondition() && A && B && C) {
        conditionNumber = 5693;
        return true;
    }
    return false;
}

...

bool evaluateCondition76() {
    if (!anyCondition() && !B && C && D) {
        conditionNumber = 7658;
        return true;
    }
    return false;
}

Therefore, if you have too many expensive checks to be performed, and things in this code remains private, this approach helps maintaining it, enabling to change the order of the checks if necessary.

clearConditions();
evaluateCondition10();
evaluateCondition9();
evaluateCondition8();
evaluateCondition7();
...
evaluateCondition34();
...
evaluateCondition76();

if (anyCondition()) { addAlert(getConditionNumber()); }

This answer just provides some alternative suggestion from the other answers, and probably will not be better than the original code if we consider only 4 lines of code. Although, this is not a terrible approach (and neither makes maintenance more difficult like others have said) given the scenario I mentioned (too many checks, only main function exposed as public, all functions are implementation details of the same class).

Answer 9

The question lacks some specifics. If the conditions are:

• subject to change or
• repeated in other parts of the application or system or
• modified in certain cases (such as different builds, testing, deployments)

or if the contents in addAlert is more complicated, then a possibly better solution in say c# would be:

//in some central spot
IEnumerable<Tuple<Func<bool>, int>> Conditions = new ... {
  Tuple.Create(CheckCondition1, 1),
  Tuple.Create(CheckCondition2, 2),
  ...
}

//at the original place
var matchingCondition = Conditions.Where(c=>c.Item1()).FirstOrDefault();
if(matchingCondition != null) 
  addAlert(matchingCondition.Item2)

Tuples are not so beautiful in c# < 8, but chosen for convienence.

The pros with this method, even if none of the options above apply, is that the structure is statically typed. You can't accidentally screw up by, say, missing an else.

Answer 10

The best way to reduce Cyclomatic complexity in cases where you have a lot of if->then statements is to use a dictionary or list (language dependent) to store the key value (if statement value or some value of) and then a value/function result.

For example, instead of (C#):

if (i > 10) { return "Two"; }
else if (i > 8) { return "Four" }
else if (i > 4) { return "Eight" }
return "Ten";  //etc etc say anything after 3 or 4 values

I can simply

var results = new Dictionary<int, string>
{
  { 10, "Two" },
  { 8, "Four"},
  { 4, "Eight"},
  { 0, "Ten"},
}

foreach(var key in results.Keys)
{
  if (i > results[key]) return results.Values[key];
}

If you're using more modern languages you can store more logic then simply values as well (c#). This is really just inline functions, but you can also just point to other functions as well if the logic is to narly to put in-line.

var results = new Dictionary<Func<int, bool>, Func<int, string>>
{
  { (i) => return i > 10; ,
    (i) => return i.ToString() },
  // etc
};

foreach(var key in results.Keys)
{ 
  if (key(i)) return results.Values[key](i);
}

Answer 11

I'm trying to follow Uncle Bob's clean code suggestions and specifically to keep methods short.

I find myself unable to shorten this logic though:

if (checkCondition()) {addAlert(1);}
else if (checkCondition2()) {addAlert(2);}
else if (checkCondition3()) {addAlert(3);}
else if (checkCondition4()) {addAlert(4);}

Your code is already too short, but the logic itself shouldn't be changed. At first glance it looks like you are repeating yourself with four calls to checkCondition(), and it's only apparent that each one is different after rereading the code carefully. You should add proper formatting and function names, e.g.:

if (is_an_apple()) {
  addAlert(1);
}
else if (is_a_banana()) {
  addAlert(2);
}
else if (is_a_cat()) {
  addAlert(3);
}
else if (is_a_dog()) {
  addAlert(4);
}

Your code should be readable above all else. Having read several of Uncle Bob's books, I believe that is the message he is consistently trying to get across.

Answer 12

Any more than two "else" clauses forces the reader of the code to go thru the whole chain to find the one of interest. Use a method such as: void AlertUponCondition(Condition condition) { switch (condition) { case Condition.Con1: ... break; case Condition.Con2: ... break; etc... } Where "Condition" is a proper enum. If needed, return a bool or value. Call it like this: AlertOnCondition(GetCondition());

It really can't get any simpler, AND it's faster than the if-else chain once you exceed a few cases.

Answer 13

I cannot speak for your particular situation because the code is not specific, but...

code like that is often a smell for a lacking OO model. You really have four type of things, each associated with its own alerter type, but rather than recognizing these entities and create a class instance for each, you treat them as one thing and try to make up for it later, at a time you really need to know what you are dealing with in order to proceed.

Polymorphism may have suited you better.

Be suspicious of code with long methods containing long or complex if-then constructs. You often want a class tree there with some virtual methods.