11

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?

closed as primarily opinion-based by durron597, Karl Bielefeldt, GlenH7, MetaFight, Bart van Ingen Schenau Jun 17 '15 at 19:18

Many good questions generate some degree of opinion based on expert experience, but answers to this question will tend to be almost entirely based on opinions, rather than facts, references, or specific expertise. If this question can be reworded to fit the rules in the help center, please edit the question.

  • 25
    The third form is ridiculous; it's just stating what should already be blatantly obvious in the second form. – Robert Harvey Jun 17 '15 at 14:44
  • 6
    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 '15 at 15:03
  • 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 '15 at 15:24
  • 4
    Options 1 and 3 say to me "The author doesn't really understand boolean logic". – 17 of 26 Jun 17 '15 at 16:19
  • 2
    Variant 1 may be useful if you need to often set a breakpoint that depends on the value. – Ian Jun 17 '15 at 16:45
40

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.

  • 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 '15 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. – user2357112 Jun 17 '15 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.

  • Well, variant 1 shares its advantage with variant 2... – Deduplicator Jun 17 '15 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 '15 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 '15 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.

  • 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 '15 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.