45

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).

  • 46
    What is actually wrong or unclear about this code in its context? I can't see how it can possibly be shortened or simplified! The code which evaluates the conditions already appears well-factored, as is the method that is called as a result of the decision. You only have to look at some of the answers below, that merely complicate the code! – Steve Jan 12 '18 at 12:41
  • 38
    There's nothing wrong with this code. It's very readable and easy to follow. Anything you do to shrink it further is going to add indirection and make it harder to understand. – 17 of 26 Jan 12 '18 at 13:01
  • 20
    Your code is fine. Put your remaining energy into something more productive than trying to shorten it further. – Robert Harvey Jan 12 '18 at 16:10
  • 5
    If it's really just 4 conditions, this is fine. If it's really something like 12 or 50 then you probably want to refactor at higher level than this one method. – JimmyJames Jan 12 '18 at 16:16
  • 9
    Leave your code exactly as it is. Listen to what your parents always told you: Don't trust any uncles offering sweets to children on the street. @Harvey Funny enough, the various attempts at "improving" the code all made it a lot larger, more complicated, and less readable. – gnasher729 Jan 12 '18 at 20:21

13 Answers 13

82

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;
}
  • 2
    If possible to encapsulate like you describe (I suspect it might not be, I think OP is leaving out variables for simplicity), it doesn't change the code, which in itself is fine, but adds code ergonomics and a bit of readability +1 – opa Jan 12 '18 at 14:15
  • 17
    With those alert codes, I thank code only one can be returned at a time – Josh Part Jan 12 '18 at 17:47
  • 12
    This seems also a perfect match for use of a switch statement - if that is available in the language of OP. – Frank Hopkins Jan 12 '18 at 19:24
  • 4
    It is probably only a good idea to extract getting the error code to a new method if that can be written so as to be useful in multiple situations without having to be given a bunch of information about the particular situation. Actually there is a trade-off and a break-even point when it is worth it. But often enough you will see that the sequence of validations is specific to the job in hand, and it is better to keep them together with that job. In such cases inventing a new type to tell another part of the code what needs to be done is undesirable ballast. – PJTraill Jan 12 '18 at 21:24
  • 6
    One issue with this reimplementation is that it makes the function addAlert need to check for the bogus alert condition AlertCode.None. – David Hammen Jan 15 '18 at 8:55
70

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.

  • 2
    I'd argue that replacing else if with an inner return followed by a simple if (removing the else) might make the code more difficult to read. When the code says else if, I immediately know that the code in the next block will only ever execute if the previous one didn't. No muss, no fuss. If it's a plain if then it might execute or it might not, irrespective of whether the previous one executed. Now I'll have to expend some amount of mental effort to parse the previous block to note that it ends with a return. I'd rather spend that mental effort on parsing the business logic. – a CVn Jan 14 '18 at 20:20
  • 1
    I know, it's a small thing, but at least to me, else if forms one semantic unit. (It's not necessarily a single unit to the compiler, but that's okay.) ...; return; } if (... doesn't; let alone if it's spread out on multiple lines. That's something I'll actually have to look at to see what it's doing, instead of being able to take it in directly by just seeing the keyword pair else if. – a CVn Jan 14 '18 at 20:21
  • @MichaelKjörling Full Ack.I would myself prefer the else if construct, especially since its chained form is such a well-known pattern. However, code of the form if(...) return ...; is also a well-known pattern, so I would not fully condemn that. I see this really as a minor issue, though: The control flow logic is the same in both cases, and a single closer look at a if(...) { ...; return; } ladder will tell me that it's indeed equivalent to an else if ladder. I see the structure of a single term, I infer its meaning, I realize that it's repeated everywhere, and I know what's up. – cmaster Jan 14 '18 at 22:57
  • Coming from JavaScript/node.js, some would use the "belt and suspenders" code of using both else if and return. e.g. else if(...) { return alert();} – user949300 Jan 15 '18 at 6:43
  • 1
    "And remember, don't get religious even about this advice as well." +1 – Words Like Jared Jan 15 '18 at 13:47
22

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
    }
}
  • 27
    This only moves the if...else ladder into the construction of the Conditions list. The net gain is negative, as the construction of Conditions will take just as much code as the OP code, but the added indirection comes with a cost in readability. I'd definitely prefer a clean coded ladder. – cmaster Jan 12 '18 at 12:06
  • 3
    @cmaster yes I think i did say exactly that "then the setup for the object will be as complicated as the original if statement£ – Ewan Jan 12 '18 at 12:23
  • 7
    This is less readable than the original. In order to figure out what condition is actually getting checked, you need to go digging in some other area of the code. It adds an unnecessary level of indirection that makes the code harder to understand. – 17 of 26 Jan 12 '18 at 13:02
  • 8
    Converting an if .. else if .. else .. chain to a table of predicates and actions makes sense, but only for much larger examples. The table adds some complexity and indirection, so you need enough entries to amortize this conceptional overhead. So, for 4 predicate/action pairs, keep the simple original code, But if you had 100, definitely go with the table. The crossover point is somewhere in between. @cmaster, the table can be statically initialized, so the incremental overhead for adding a predicate/action pair is one line that merely names them: hard to do better. – Stephen C. Steel Jan 12 '18 at 14:31
  • 2
    Readability is NOT personal. It is a duty to the programming public. It is subjective. Which is exactly why it's important to come to places like this and listen to what the programming public has to say about it. Personally I find this example incomplete. Show me how conditions is constructed... ARG! Not annotation-attributes! Why god? Ow my eyes! – candied_orange Jan 12 '18 at 15:56
7

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.

  • 3
    Of course, this assumes that nothing else happens after the chain of ifs... That might be a reasonable assumption, and then again it might not be. – a CVn Jan 14 '18 at 20:23
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.

  • 1
    ternary is neat – Ewan Jan 13 '18 at 13:29
  • 6
    @Ewan debugging a broken ”deeply recursive ternary” can be an unecessary pain. – dfri Jan 13 '18 at 15:02
  • 5
    it looks neat on the screen though. – Ewan Jan 13 '18 at 15:13
  • Uhm, what language allows using functions with case labels? – undercat Jan 14 '18 at 19:36
  • 1
    @undercat that's a valid ECMAScript/JavaScript afaik – zworek Jan 15 '18 at 10:22
1

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);
  • Yes that's called the Chain-of-responsibility Pattern – Max Jan 12 '18 at 16:55
  • 4
    I won't pretend to speak for anyone else, but while the code in the question is immediately readable and obvious in its behavior, I would not consider this to be immediately obvious as to what it does. – a CVn Jan 14 '18 at 20:25
0

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.

  • 1
    (Did not downvote but) I cannot follow this. What is head? – Belle-Sophie Jan 12 '18 at 12:15
  • @Belle I edited the answer to explain further. It's the same idea as Ewan's but with a while not instead of foreach break. – Nico Jan 12 '18 at 12:27
  • A brilliant evolution of a brilliant idea – Ewan Jan 12 '18 at 16:11
0

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.

0

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);
}
0

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.

0

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).

  • I don't like this suggestion - it hides the testing logic inside multiple functions. This can make the code hard to maintain if, for example, you needed to change the order and do #3 before #2. – Lawrence Jan 13 '18 at 17:14
  • No. You can check if some previous condition was evaluated if anyCondition() != false. – Emerson Cardoso Jan 13 '18 at 19:27
  • 1
    Ok, I see what you're getting at. However, if (say) conditions 2 and 3 both evaluate to true, the OP doesn't want condition 3 evaluated. – Lawrence Jan 14 '18 at 8:46
  • What I meant is that you can check anyCondition() != false within functions evaluateConditionXX(). This is possible to implement. If the approach of using internal state is not desired I understand, but the argument that this does not work is not valid. – Emerson Cardoso Jan 14 '18 at 22:44
  • 1
    Yes, my objection is that it unhelpfully hides the testing logic, not that it can't work. In your answer (paragraph 3), the check for meeting condition 1 is placed inside eval...2(). But if he switches conditions 1 and 2 at the top level (due to changes in customer requirements, etc), you'd have to go into eval...2() to remove the check for condition 1, plus go into eval...1() to add a check for condition 2. This can be made to work, but it can easily lead to problems with maintenance. – Lawrence Jan 15 '18 at 1:53
0

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.

0

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.

protected by gnat Aug 20 '18 at 10:23

Thank you for your interest in this question. Because it has attracted low-quality or spam answers that had to be removed, posting an answer now requires 10 reputation on this site (the association bonus does not count).

Would you like to answer one of these unanswered questions instead?

Not the answer you're looking for? Browse other questions tagged or ask your own question.