3

Developers are sometimes using following syntax in code,

if (isProductFlag == true) 

if (customerExist == true)  
..etc

Sonarqube is giving warning. The question is doing above syntax have negative affects, meaning affect on speed/performance, causing errors, largely increase compilation time, bugs in the system? Anything to watch out for which can be harmful to program? Considering turning this particular Sonarqube warning off, if no harm done. We know above syntax is superfluous, it should be if (isProductFlag) or if (customerExist). However, every programmer has their style. Lot of other issues in program we can focus on with deadlines, etc.

SonarQube: Remove the unnecessary Boolean literal(s).

Resource: Is it bad to explicitly compare against boolean constants e.g. if (b == false) in Java?

Update: How does this work with nullable booleans?

2 Answers2

4

The Roslyn compiler optimises such literals anyway. It will prune unreachable code, perform computations on compile-time constants where it can, etc. It's safe to say there is no impact on the generated code, and you can witness that with a decompiler - the IL is identical with or without redundant constants.

SonarQube is giving you a warning for two reasons. One, this code is unnecessarily verbose. Clean code practices advocate that code should be readable, preferably closely to actual natural language. Which one reads better?

if (customerExists)
{
    DoWork();
}

if (customerExists == true)
{
    DoWork();
}

"If customerExists, DoWork" or rather "If customerExists equals true, DoWork"? Which one is more likely to appear as a customer requirement in a design document? No one speaks like "If it rains equals true, I'll take an umbrella".

Two, a line like this is vexing. Why is the developer explicitly stating that this is supposed to equal to true? Is there something else going on that I can't see? I'd probably mouse over the variable to make sure that it's even a boolean. If something is unnecessary, it immediately raises a question of "why is that here?". You want to avoid your developers asking such questions. The intention should be clear.

I think I should add that the latter is arguably nastier. This is something that Andy Hunt and Dave Thomas called "a broken window" in their excellent "Pragmatic Programmer". Such little things build a feeling in the developers that no one really cares about the code. Is that thing necessary? Damned if I know, someone wrote it ages ago, better leave it as it is. It starts with small things and builds up over time. Might seem like pedantry at first, and I'm certainly not trying to doomsday-talk you into thinking that your whole project is going to crash and burn because of a few boolean literals. But if no one cares about this aspect of the code, why should they care about the next one?

V0ldek
  • 9,623
  • 1
  • 26
  • 57
  • @madreflection You can even ["hack" booleans](https://blogs.msmvps.com/bill/2004/06/22/a-hacked-boolean/) into being neither true nor false in .NET, which is fun. Also, my crazy mind immediately imagined a custom type with an overloaded [true operator](https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/operators/true-false-operators), but with differently overloaded equality comparison with booleans, so that if(x) is different from if(x == true). – V0ldek Jan 07 '20 at 23:25
  • 3
    I will sometimes compare to boolean literals, but only because I'm comparing a `bool?` and want to concisely handle null values. – Jeremy Lakeman Jan 07 '20 at 23:46
  • Although Broken Windows is not a decided effective tool. The jury is still out. And on the flip side, overly pedantic code practices are just as bad in other ways. – Austin T French Jan 08 '20 at 16:23
  • @Artportraitdesign1 [There's a question for that](https://stackoverflow.com/questions/2673918/best-way-to-check-for-nullable-bool-in-a-condition-expression-if). – V0ldek Jan 09 '20 at 13:18
1

It's purely style, but suggests your code isn't very clear:

if (productFlag == true)

is superfluous and would be better served with better variable names:

if (hasProductFlag)
{
   //...
}

this also makes refactoring clearer if later required as

if (hasProductFlag())

is still more clear than

if (hasProductFlag() == true)

Is there a fucntional difference? Almost certainly not. But it could reduce bugs from being more clear.

It is feasible that the compiled code might rarely compile slightly differently. But there should never be a functional difference because of it.

Austin T French
  • 5,022
  • 1
  • 22
  • 40
  • well, my variabe names are pretty clearn, just looking for negative effects, we have team of 40 developers, only want to focus on real bugs, errors, and code issues which really affect system, performance, thank you for this though –  Jan 07 '20 at 23:04
  • yeah, you mentioned exactly what I stated in bottom of question resolution, we're just trying to mitagate perfect code with all other issues in system –  Jan 07 '20 at 23:06
  • @Artportraitdesign1 then I'd append (and will) that the only real difference will likely be a very rare compiled code difference. Performance / functional difference would be none. – Austin T French Jan 07 '20 at 23:08
  • now would the code compilation issue be large to watch out for? if not, I'll allay the rule, I can find multiple examples in our team where can review true compilation performance issues, thanks for the input –  Jan 07 '20 at 23:09
  • @Artportraitdesign1 no, they would be so small only someone comparing the application written both ways (resharper for example) might notice. But it would only be by using diff tools or looking way to closely at the wrong the things. – Austin T French Jan 07 '20 at 23:11
  • @Artportraitdesign1 that depends on what a null boolean means in that business context. ` if (hasProductFlag.GetValueOrDefault()) ` would suit my designs. Which would be the same as verbosely writting ` if (myBool != null && myBool == true) ` – Austin T French Jan 09 '20 at 15:47