2

In our project there are deep and lengthy if-else structures which can be significantly shortened by a simple rearrangement. To me the shorter version would be easier to understand also.

As an example, this:

if(a) {
    if(b) {
        doSomething1;
    }
    else {
        doSomething2;
    }
}
else {
    if(b) {
        doSomething1;
    }
    else {
        if(c) {
            doSomething3;
        }
        else {
            doSomething2;
        }
    }
}

could be refactored to:

if(b) {
    doSomething1;
}
else {
    if(!a && c) {
        doSomething3;
    }
    else {
        doSomething2;
    }
}

Is it worth to do this kind of rearrangements?

Is there a name for this kind of "rearranging" if else structures to shorter form?

I am looking for arguments to convince colleges that it is worth to take the effort to do the changes. Or - of course - arguments against it, to convince me that it is not worth to do this.

I think I can agree with Ewan that it's probably not worth to change working code for this.

However if it's new code or we are doing a major refactor (and proper re-testing of every execution path is guaranteed) then I would shorten it like above, OR -- especially if there is no way to significantly shorten the structure -- go for the "self documenting logic table" solution pointed out by David:

if     ( a &&  b &&  c) doSomething1;
else if( a &&  b && !c) doSomething1;
else if( a && !b &&  c) doSomething2;
else if( a && !b && !c) doSomething2;
else if(!a &&  b &&  c) doSomething1;
else if(!a &&  b && !c) doSomething1;
else if(!a && !b &&  c) doSomething3;
else if(!a && !b && !c) doSomething2;

Besides being self documented, I think subsequent changes will be easier to follow in code revisioning system logs for this too.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
riskop
  • 129
  • 4
  • 1
    please put { on new lines! for me it is easier to understand – Ewan Oct 17 '18 at 10:54
  • 3
    ... or not? That's a matter of preference; those newlines eat up a lot of vertical space which could be used to show more code. (At least, that is my personal opinion - I know this debate can be at least as intense as tabs vs. spaces.) – Glorfindel Oct 17 '18 at 11:30
  • This kind of rearrangement is generally "refactoring" by the way. It's the generic term for code changes that are supposed to change (and presumably improve) the code in some way, without introducing functional changes. – Useless Oct 17 '18 at 13:44
  • If *a,b,c * have meaning names, more branches might seem ugly or code-smell, but it doesn't matter if the code, as a whole, is readable and understandable. For efficiency, I would rather first ask myself what's the frequency of execution for this block. If it's executed million times within a loop, then an optimization might help. If it's going to be executed once every year, I would not care. – Laiv Oct 17 '18 at 14:26

4 Answers4

1

If in doubt, prefer brevity.
If there is less code to understand, it will be easier on the reader, assuming roughly the same code-quality. And quality was not sacrified there.

But be aware that doing any change risks introducing new defects, so do you have it covered by automated tests?
Improving test-coverage might be a better use of your time, as might be adding new features, or doing more substantial refactorings.
Do you work on that code at all anymore, which includes just reading to understand it, maybe while hunting bugs elsewhere?

All else being equal (it never is), I would go with something like the second option, after cleaning up the superfluous block and making it a proper conditional chain.

Still, refactoring would lead me to have the last two blocks in a different order. I wonder why you reversed them:

if(b) {
    doSomething1;
}
else if(a || !c) {
    doSomething2;
}
else {
    doSomething3;
}

Personally, I would put else on the same line as the closing brace, but do as you will (or must), consistency is paramount, and there is no use in starting yet another flamewar about the one and only true style.

Beware of evaluating a, b, or c possibly having side-effects which must be considered though.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
0

Yes. You have a case. Refactor it, assuming it's low risk/it's covered with unit tests. To be honest things like that should be caught during code review - a good developer should seek a reasonable minimal level of expression for a given problem and it's pretty obvious that's not been done here.

I don't think there is a name for it other than it's not DRY (Don't repeat yourself) - the same 'if' statement is written out twice.

Obviously don't refactor it for its own sake - just do it next time you are in that code area doing something. In fact, if I saw code like that I'd be aggressive:

  • move the sub-conditions into their own functions.
  • see if the doSomethings have common code this is shared and rejig as appropriate.
  • recheck against software requirements that all the doSomethings are actually needed.
  • eliminate the need for ifs, by factoring out the doSomethings into behaviour classes/use dynamic dispatch. Add unit tests for those.

I'm not in support of a truth table in this case as you only have the two simple conditions. If it's not performance critical what might work for you is to evaluate the conditions and store to named bools, then use the bools in the ifs later. It's a little bit more self-documenting: e.g.

bool is_enabled = ...;
if (is_enabled) {
Benedict
  • 1,047
  • 5
  • 12
-1

Short answer "No".

Long answer "Refactoring for style is never worth it".

Every change you make comes with risk of breaking something. To be worth the risk, there has to be some potential monetary gain. Usually this is a new feature, but it might be improved performance or some other non functional.

Style changes don't add features. The argument that it speeds up future development is very tenuous. Doubly so if the development you are speeding up has a large percentage of style changes.

PS Personally, I think the first example is way more readable than the second. Even though the duplicated line is irksome.

Ewan
  • 70,664
  • 5
  • 76
  • 161
  • It's more than one duplicated line. Even if those placeholders were only for one line each. – Deduplicator Oct 17 '18 at 12:33
  • @Deduplicator sure, maybe. I still wouldn't refactor working code though – Ewan Oct 17 '18 at 13:07
  • If you have no need to look at the file at all, sure, than *any* improvement is not significant *enough*. But if you do, reducing size by half or more without getting tricky tends to be worthwhile. – Deduplicator Oct 17 '18 at 13:13
  • @Deduplicator if you are adding a feature that changes the question slightly, but the argument is the same. change as little as possible. so yes, extract the code you are changing to a function so you only have to change it one place. no, don't rearrange the whole if structure to make it look pretty – Ewan Oct 17 '18 at 13:17
  • In case where many other developers reading this code and if style change will save another 10 seconds to understand what happens inside, in that case style change can be worth – Fabio Oct 18 '18 at 19:21
-1

I'd argue for a completely different approach that removes the if's completely.

Let's say we have a=false, b=false, c=true, which function will be called? To know, we have to laboriously execute much of the nested if-structure in our heads to arrive at the answer: doSomething1. To help, we could put the logic table in a comment:

/*
  +-------+-------+-------+--------------+
  |   a   |   b   |   c   |   function   |
  +-------+-------+-------+--------------+
  | false | false | false | doSomething2 |
  | false | false | true  | doSomething3 |
  | false | true  | false | doSomething1 |
  | false | true  | true  | doSomething1 |
  | true  | false | false | doSomething2 |
  | true  | false | true  | doSomething2 |
  | true  | true  | false | doSomething1 |
  | true  | true  | true  | doSomething1 |
  +-------+-------+-------+--------------+
*/

but then we risk the code and comment getting out of sync. So put the logic table in the code. For example, in C#, I could use a dictionary:

private static readonly Dictionary<(bool, bool, bool), Action> FunctionChooser =
    new Dictionary<(bool, bool, bool), Action>
    {
        [(false, false, false)] = doSomething2,
        [(false, false, true)] = doSomething3,
        [(false, true, false)] = doSomething1,
        [(false, true, true)] = doSomething1,
        [(true, false, false)] = doSomething2,
        [(true, false, true)] = doSomething2,
        [(true, true, false)] = doSomething1,
        [(true, true, true)] = doSomething1
    };

Now that entire if - else sequence can be replaced with just one line:

FunctionChooser[(a, b, c)]();

And, in future, if I need to add a doSomething4 condition, I just edit the logic table (dictionary) to add it, rather than having to try and modify a load of nested `if's without breaking anything.

Deduplicator
  • 8,591
  • 5
  • 31
  • 50
David Arno
  • 38,972
  • 9
  • 88
  • 121
  • You know `doSomethingX` is a placeholder for any amount of code using any number of the local variables, and maybe also returning? – Deduplicator Oct 17 '18 at 12:37
  • @Deduplicator just as `Action`is a placeholder for any sort of delegates... – David Arno Oct 17 '18 at 12:51
  • sure, but would you refactor the long and complex existing code to your new style and risk an error when it otherwise requires no changes? – Ewan Oct 17 '18 at 13:13
  • Also, this is liable to be a bit less efficient. Who knows whether that's significant? In any case it's more verbose. – Deduplicator Oct 17 '18 at 13:17
  • 1
    @Ewan, I wouldn't write long and complex existing code in the first place and I *always* write unit tests to ensure I can make major refactoring changes safe in the knowledge that if I break things my tests will tell me. Of course not everyone does this, which is why they end up in the "don't refactor unless you have to" situation that you describe in your answer. – David Arno Oct 17 '18 at 13:20
  • @Deduplicator, "*Also, this is liable to be a bit less efficient*". You now appear to be prematurely optimising. – David Arno Oct 17 '18 at 13:21
  • 1
    @DavidArno of course! the same with my code! but how to write it in the first place isn't the question. – Ewan Oct 17 '18 at 13:23
  • 2
    @DavidArno Refraining from writing needlessly inefficient code is not *premature optimization*. It's just not writing inefficient code for no reason at all. – Deduplicator Oct 17 '18 at 13:23
  • 1
    @Deduplicator, it all depends on what you mean by *inefficient*. I write code for other people to read first and the compiler to process second. So I choose code that is efficient when it comes to reasoning over efficient to execute and only degrade it to computationally efficient if profiling shows it's a bottleneck. Anything else is always premature optimisation. – David Arno Oct 17 '18 at 13:27
  • Well, more verbose tends to be harder to understand. And in this case, I don't see the table-driven approach being the simpler one. Anyway, I'm bowing out now. – Deduplicator Oct 17 '18 at 13:32
  • 1
    More verbose does _tend to_ be harder to understand. But in cases like this where the more verbose code is _not_ harder to understand, you can't still use verbosity as an argument against it. That's just saying verbosity is sometimes worse so we should never be verbose even when it's actually better. – Useless Oct 17 '18 at 13:40