Search code examples
c#.net.net-coresonarqube

C# Remove the unnecessary Boolean literal, Harmful effects or Consequences?


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?


Solution

  • 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?