How could this manner of writing code be called?

Question

I'm reviewing a quite old project and see code like this for the second time already (C++ - like pseudocode):

if( conditionA && conditionB ) {
   actionA();
   actionB();
} else {
   if( conditionA ) {
      actionA();
   }
   if( conditionB ) {
      actionB();
   }
}

in this code conditionA evaluates to the same result on both computations and the same goes for conditionB. So the code is equivalent to just:

if( conditionA ) {
   actionA();
}
if( conditionB ) {
   actionB();
}

So the former variant is just twice more code for the same effect. How could such manner (I mean the former variant) of writing code be called?

Answer 1

I would call it bad code. Though I've tended to find similar constructs in project that grew without any code review being done. (Or other lax development practices).

Answer 2

I would call it bad code too.

There is no best way on indenting, but there is one golden rule : choose one and stick with it.

Answer 3

It's code written by someone who doesn't know how to use a Karnaugh Map.

Answer 4

This is "redundant" code, and yes it is bad. If some precondition must be added to the calls to actionA (assuming that the precondition can't be put into actionA itself), we now have to modify the code in 2 places, and therefore run the risk of overlooking one of them.

This is one of those times where you can feel better about deleting some lines of code, than in writing new ones.

Answer 5

This is indeed bad coding practice, but be warned that if condition A and B evaluations have any side effects (var increments, etc.) the two fragments are not equivalent.

Answer 6

inefficient code?
Also, could be called Paid per line

Answer 7

I would call it 'twice is better'. It's made like that to be sure that the runtime really understood the question ;).

(although in multi-threaded, not-safe environment, the result may differ between the two variants.)

Answer 8

I might call it "code I wrote last month while I was in a hurry / not focused / tired". It happens, we all make or have made these kind of mistakes. Just change it. If you want to you can try and find out who did this, hope it is not you, and give him/her feedback.

Answer 9

Since you said you've seen this more than once, it seems that it's more than a one-time error due to being tired. I see several reasons for someone to repeatedly come up with such code:

1. The code was originally different, got refactored, but whoever did this oversaw that this is redundant.
2. Whoever did this didn't have a good grasp of boolean logic.

(Also, there's the slight possibility that there might be more to this than what your simplified snipped shows.)

Answer 10

Guys? Look at this part: ( conditionA && conditionB ) Basically, if conditionA happens to be false, then it won't evaluate conditionB.

Now, it would be a bad coding style but if conditionA and conditionB aren't just evaluating data but if there's also some code behind these conditions that change data, there could be a huge difference between both notations!

if conditionA is false then conditionA is evaluated twice and conditionB is evaluated just once. If conditionA is true and conditionB is false, then both conditions are evaluated twice. If both conditions are true, both are executed just once.

In the second suggestion, both conditions are executed just once... Thus, these methods are only equivalent if both methods evaluate to true.

To make things more complex, if conditionB is false then actionA could change something that would change this validation! Thus the else branch would then execute actionB too. But if both conditions evaluates to true and actionA would change the evaluation of conditionB to false, it would still execute actionB.

I tend to refer to this kind of code as: "Why make things easy when you can do it the hard way?" and consider this a design pattern. Actually, it's "Mortgage-Driven development" where code is made more complex so the main developer will be the only one to understand it, while other developers will just become confused and hopefully give up to redesign the whole thing. As a result, the original developer is required to stay just to maintain this code, which is called "Job security" and thus be able to pay his mortgage for many, many years.

---

I wonder why something like this would be used, then realized that I use a similar structure in my own code. Similar, but not the same:

if (A&&B){
  action1;
} elseif(A){
  action2;
} elseif(B){
  action3;
} else{action4}

In this case, every action would be the display of a different message. A message that could not be generated by just concatenating two strings. Say, it's a part of a game and A checks if you have enough energy while B checks if you have enough mass. Of you don't have enough mass AND energy, you can't build anything anymore and a critical warning needs to be displayed. If you only have energy, a warning that you have to find more mass would be enough. With only energy, your builders would need to recharge. And with both you can continue to build. Four different actions, thus this weird construction. However, the sample in the Q shows something completely different. Basically, you'd get one message that you're out of mass and another that you're out of energy. These two messages aren't combined into a single message.

Now, in the example, if conditionA would detect energy levels and conditionB would detect mass levels then both solution would just work fine. Now, if actionA tells your builders to drop their mass and start recharging, you'd suddenly gain a little bit of mass again in your game. But if conditionB indicated that you ran out of mass, that would not be true anymore! Simply because actionA released mass again. if actionB would be the command to tell builders to start collecting mass as soon as they're able then the first solution will give all builders this command and they would start collecting mass first, then they would continue their other actions. In the second solution, no such command would be given. The builders are recharged again and start using the little mass that was just released. If this check is done every 5 minutes then those builders would e.g. recharge in one minute to be idle for 4 more minutes because they ran out of mass. In the first solution, they would immediately start collecting mass.

Yeah, it's a stupid example. Been playing Supreme Commander again. :-) Just wanted to come up with a possible scenario, but it could be improved a lot!...

Answer 11

This is very close to the 'fizzbuzz' Design Pattern:

if( fizz && buzz ) {
   printFizz();
   printBuzz();
} else {
   if( fizz ) {
      printFizz();
   }
   else if( buzz ) {
      printBuzz();
   }
   else {
      printValue();
   }
}

Maybe the code started life as an instance of fizzbuzz (maybe copied-n-pasted), then was refactored slightly into what you see today due to slightly different requirements, but the refactoring didn't go as far as it probably should have (boolean logic can sometimes be a bit trickier than one might think - hence fizzbuzz as an interview weed-out technique).

Answer 12

As pgast has said in a comment there is nothing wrong with this code if actionA effects conditionB (note that this is not a condition with a side effect but an action with a side effect (which you kind of expect))