Best practice boolean assignment [closed]

Question

Closed. This question is opinion-based. It is not currently accepting answers.

---

Want to improve this question? Update the question so it can be answered with facts and citations by editing this post.

Closed 5 years ago.

Improve this question

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?

Answer 1

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.

Answer 2

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.

Answer 3

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.

Answer 4

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.

Answer 5

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 World^TM 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.

Answer 6

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.