How to explain to a developer that adding extra if - else if conditions is not a good way to "improve" readability?

Recently I've bumped into the following C++ code:

if (a)
{
  f开发者_Go百科();
}
else if (b)
{
  f();
}
else if (c)
{
  f();
}

Where a, b and c are all different conditions, and they are not very short.

I tried to change the code to:

if (a || b || c)
{
  f();
}

But the author opposed saying that my change will decrease readability of the code. I had two arguments:

1) You should not increase readability by replacing one branching statement with three (though I really doubt that it's possible to make code more readable by using else if instead of ||).

2) It's not the fastest code, and no compiler will optimize this.

But my arguments did not convince him.

What would you tell a programmer writing such a code?

Do you think complex condition is an excuse for using else if instead of OR?

---

This code is redundant. It is prone to error.

If you were to replace f(); someday with something else, there is the danger you miss one out.

---

There may though be a motivation behind that these three condition bodies could one day become different and you sort of prepare for this situation. If there is a strong possibility it will happen, it may be okay to do something of the sort. But I'd advice to follow the YAGNI principle (You Ain't Gonna Need It). Can't say how much bloated code has been written not because of the real need but just in anticipation of it becoming needed tomorrow. Practice shows this does not bring any value during the entire lifetime of an application but heavily increases maintenance overhead.

---

As to how to approach explaining it to your colleague, it has been discussed numerous times. Look here:

How do you tell someone they’re writing bad code?

How to justify to your colleagues that they produce crappy code?

How do you handle poor quality code from team members?

“Mentor” a senior programmer or colleague without insulting

---

Replace the three complex conditions with one function, making it obvious why f() should be executed.

bool ShouldExecute; { return a||b||c};
...
if ShouldExecute {f();};

---

Since the conditions are long, have him do this:

if ( (aaaaaaaaaaaaaaaaaaaaaaaaaaaa)
  || (bbbbbbbbbbbbbbbbbbbbbbbbbbbb)
  || (cccccccccccccccccccccccccccc) )
{
    f();
}

A good compiler might turn all of these into the same code anyway, but the above is a common construct for this type of thing. 3 calls to the same function is ugly.

---

In general I think you are right in that if (a || b || c) { f(); } is easier to read. He could also make good use of whitespace to help separate the three blocks.

That said, I would be interested to see what a, b, c, and f look like. If f is just a single function call and each block is large, I can sort of see his point, although I cringe at violating the DRY principle by calling f three different times.

---

Performance is not an issue here.

Many people wrap themselves in the flag of "readability" when it's really just a matter of individual taste.

Sometimes I do it one way, sometimes the other. What I'm thinking about is -

"Which way will make it easier to edit the kinds of changes that might have to be made in the future?"

Don't sweat the small stuff.

---

I think that both of your arguments (as well as Developer Art's point about maintainability) are valid, but apparently your discussion partner is not open for a discussion.

I get the feeling that you are having this discussion with someone who is ranked as more senior. If that's the case, you have a war to fight and this is just one small battle, which is not important for you to win. Instead of spending time arguing about this thing, try to make your results (which will be far better than your discussion partner's if he's writing that kind of kode) speak for themselves. Just make sure that you get credit for your work, not the whole team or someone else.

This is probably not the kind of answer you expected to the question, but I got a feeling that there's something more to it than just this small argument...

---

I very much doubt there will be any performance gains of this, except at least in a very specific scenario. In this scenario you change a, b, and c, and thus which of the three that triggers the code changes, but the code executes anyhow, then reducing the code to one if-statement might improve, since the CPU might have the code in the branch cache when it gets to it next time. If you triple the code, so that it occupies 3 times the space in the branch cache, there is a higher chance one or more of the paths will be pushed out, and thus you won't have the most performant execution.

This is very low-level, so again, I doubt this will make much of an impact.

As for readability, which one is easier to read:

• if something, do this
• if something else, do this
• if yet another something else, do this
• "this" is the same in all three cases

or this:

• if something, or something else, or yet another something else, then do this

Place some more code in there, other than just a simple function call, and it starts getting hard to identify that this is actually three identical pieces of code.

Maintainability goes down with the 3 if-statement organization because of this.

You also have duplicated code, almost always a sign of bad structure and design.

Basically, I would tell the programmer that if he has problems reading the 1 if-statement way of writing it, maybe C++ is not what he should be doing.

Now, let's assume that the "a", "b", and "c" parts are really big, so that the OR's in there gets lost in lots of noise with parenthesis or what not.

I would still reorganize the code so that it only called the function (or executed the code in there) in one place, so perhaps this is a compromise?

bool doExecute = false;
if (a) doExecute = true;
if (b) doExecute = true;
if (c) doExecute = true;
if (doExecute)
{
    f();
}

or, even better, this way to take advantage of boolean logic short circuiting to avoid evaluating things unnecessary:

bool doExecute = a;
doExecute = doExecute || b;
doExecute = doExecute || c;
if (doExecute)
{
    f();
}

---

Performance shouldn't really even come into question

Maybe later he wont call f() in all 3 conditons

---

Repeating code doesn't make things clearer, your (a||b||c) is much clearer although maybe one could refactor it even more (since its C++) e.g.

x.f()