10

I came across the following conditional in a program that I have taken over from another developer:

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

I believe this code is redundant and ugly, so I changed it to what I thought was a simple boolean assignment based on a comparison:

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE;

Upon seeing this, someone reviewing my code commented that although my change is functionally correct, it might confuse someone else looking at it. He believes that using a ternary operator makes this assignment more clear, whereas I don't like the addition of more redundant code:

obj.NeedsChange = (obj.Performance <= LOW_PERFORMANCE) ? true : false;

His reasoning is that doing something in the most concise way is not worth it, if it causes another developer to have to stop and puzzle out exactly what you've done.

The real question here is which of these three methods of assigning a value to the boolean obj.NeedsChange is the most clear and the most maintainable?

13
  • 26
    The third form is ridiculous; it's just stating what should already be blatantly obvious in the second form. Jun 17, 2015 at 14:44
  • 7
    This is entirely up to personal preference. We can pretend otherwise, but because they're all functionally equivalent it boils down to style. Sure, there's a difference in readability, but my "readable and transparent" might be your "obtuse and opaque"
    – MetaFight
    Jun 17, 2015 at 15:03
  • 3
    @MetaFight 5-8 lines vs 1 line is not just a personal preference. There's more room for typos, which matters if it's an interpreted language. Also it sounds more like a excuse of not understanding of "expression vs statement" topic.
    – scriptin
    Jun 17, 2015 at 15:19
  • 3
    @scriptin 5-8 lines v 1 line is more than preference, the 5-8 liner is usually clearer and better for it. In this simple example, I prefer the 1 line, but in general I've seen too many 10 liners that were obfuscated into 1-liners for comfort. Given that, I'd never complain about variant 1, it may not be pretty but it does the job just as clearly and obviously.
    – gbjbaanb
    Jun 17, 2015 at 15:24
  • 6
    Options 1 and 3 say to me "The author doesn't really understand boolean logic".
    – 17 of 26
    Jun 17, 2015 at 16:19

6 Answers 6

39

I prefer 2, but I might go for a small adjustment to it:

obj.NeedsChange = ( obj.Performance <= LOW_PERFORMANCE );

To me the parentheses makes the line easier to parse and makes it clear at a glance that you are assigning the result of a comparison, and not performing a double assignment. I'm not sure why that is (as off-hand I can't think of a language where parentheses would actually prevent a double assignment), but if you must satisfy your reviewer then perhaps this will be an acceptable compromise.

2
  • 4
    this is the correct answer - although the code in the question is correct, adding the brackets does tell the reader that it is not as assignment. If you were quickly looking through code, the brackets give you that instant extra info that stops you looking closer to see if the code meant to be like that, and wasn't an accidental bug. For example, imagine the line was a = b == c, did you mean assign a bool or did you mean assign c to both b and a.
    – gbjbaanb
    Jun 17, 2015 at 15:26
  • Parentheses would prevent a double assignment in Python. Even in languages where they don't prevent double assignment, the parentheses definitely help indicate that you're dealing with two kinds of operations. Jun 17, 2015 at 18:15
23

Variant 1 is easily understood, but that is its only advantage. I automatically assume that anyone who writes like this doesn't really understand what booleans are all about, and will be writing similarly infantile code in many other respects.

Variant 2 is what I would always write, and expect to read. I think that anyone who is confused by that idiom shouldn't be a professional writer of software.

Variant 3 combines the disadvantages of both 1 and 2. 'nuff said.

3
  • Well, variant 1 shares its advantage with variant 2... Jun 17, 2015 at 16:56
  • 1
    +1 for infantile code. I've been looking at such code for years, I just lacked the right word to identify it.
    – Lilienthal
    Jun 17, 2015 at 18:22
  • 1
    My first assumption with code like Variant 1 is that the two branches at one point in the past were more complicated, and someone wasn't paying attention when refactoring. If however that's how it was when first written, then I agree with "not understanding booleans"
    – Izkata
    Jun 17, 2015 at 18:30
13

Anytime code is more complicated than it needs to be triggers a "what is this supposed to be doing?" smell in the reader.

For example, the first example makes me wonder, "was there other functionality in the if/else statement at some point that was removed?"

Example (2) is simple, clear, and does exactly what is needed. I read it and immediately understand what the code does.

The extra fluff in (3) would cause me to wonder why the author wrote it that way instead of (2). There should be a reason, but in this case there doesn't seem to be, so it's not helpful at all and harder to read because the syntax suggests something present which is not there. Trying to learn what is present (when nothing is) makes code harder to read.

2

It is easy to see that Variant 2 and Variant 1 are related via a series of obvious and simple refactorings:

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

Here, we have needless code duplication, we can factor out the assignment:

obj.NeedsChange = if (obj.Performance <= LOW_PERFORMANCE)
{
    true
}
else
{
    false
}

or written more concisely:

obj.NeedsChange = if (obj.Performance <= LOW_PERFORMANCE) true else false

Now, it should be immediately obvious that this will assign true if the condition is true and assign false if the condition is false, IOW it will simply assign the value of the condition, i.e. it is equivalent to

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE

Variants 1 and 3 are typical rookie code written by someone who doesn't understand what the return value of a comparison is.

1
  • I would add your if (...)...false part as a comment just before the nice one, then you get the simple scan through the code clarity and the better code.
    – DaveM
    Jun 17, 2015 at 17:57
2

While your programming should tend towards explicit over implicit "as if the guy who ends up maintaining your code will be a violent psychopath who knows where you live", you can assume a few basic things that your psycho successor will be competent in.

One of those is the syntax of the language he is using.

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE;

is very clear to anyone who knows C/C++/C#/Java/Javascript syntax.

It's also much more readable than 8 lines.

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.NeedsChange = false;
}

and less prone to mistakes

if (obj.Performance <= LOW_PERFORMANCE)
{
    obj.NeedsChange = true;
}
else
{
    obj.Needschange = false;
}

And it's better than adding unnecessary characters, as if you half-forgot the syntax of the language:

obj.NeedsChange = obj.Performance <= LOW_PERFORMANCE ? true : false;

obj.NeedsChange = (obj.Performance <= LOW_PERFORMANCE);

I think there is much wrong about the C-like syntax of many languages: inconsistent order of operations, inconsistent left/right associativity, overloaded uses of symbols, braces/indentation duplication, ternary operators, infix notation, etc.

But the solution isn't to invent your own proprietary version of it. That way lies madness, as everyone creates their own.


Generally the #1 thing that makes Real WorldTM code unreadable is the amount of it.

Between a non-pathological 200 line program and an trivially identical 1,600 line program, the shorter one will almost always be easier to parse and understand. I would welcome your change any day.

1

Most of the developers will be able to understand the 2nd form with a glance. In my opinion over simplification as in 1st form is simply unnecessary.

You can improve the readability by adding spaces and braces such as:

obj.NeedsChange =    obj.Performance <= LOW_PERFORMANCE;

or

obj.NeedsChange = ( obj.Performance <= LOW_PERFORMANCE );

as mentioned by Jacob Raihle.

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