Boolean literals should not be redundant C# csharpsquid : S1125

My suggestion to remove an example#1 style as not a Noncompliant Code. For csharpsquid : S1125
With title Boolean literals should not be redundant

Example #1 (flagged as a Code Smell)
if (booleanMethod() == false) { /* ... */ }

Let’s compare this to the compliant example of this code.

Example #2 (Suggested Fix)
if (!booleanMethod()) { /* ... */ }

My paradigm for challenging this is summed up in this line from the book Clean Code pg.8 which states

“Clean code reads like well-written prose”

.
now let’s compare examples when reading that code as plain English prose

#1 if method result is false.
#2 if not method result.

I assert that “if method result is false.” is much more natural than “if not method result”.

“if not method result” requires a bit of mental gymnastics it reason about. First, I want to understand what the method result is. Second, I need to change the value of the method result into its opposite. Converting True to False and False to true.

furthermore a simple ! can easily read over. eg !earn() can read as learn() but earn() == false is unlikely to be confused the same way.

Therefore

Example 1 is more readable then Example #2 in C#.

Action request
As Sonar Cloud user I would like the code matching the example of if (booleanMethod() == false) { /* ... */ } Not to be alearted on.

thank you for time first post hear still learning the rules i welcome feedback.

Hello Luke,

I would disagree on this one. Take a look at these more basic examples:

Example #3

if(booleanMethod()) {...}

Example #4

if(booleanMethod() == true) {...}

This translates into English prose

#3 if method result.
#4 if method result is true.

Looking at this, the #4 is way more natural than #3. But from source code point of view, #4 is a bad practice and clear violation of rule S1125.

If we update methodResult() to something else

Example #5

if(!Valid) {...}

Example #6

if(Valid == false) {...}

we get another set of proses:

#5 if not valid
#6 if valid is false

In this case, #5 is better than #6 in English, but not in code.

Those true/false literal are unnecessary, just as rule suggests.

You can create your own quality profile in SonarCloud and exclude this rule if you feel it too noisy for your code base.

Pavel Mikula

First of all, let me say that I completely agree with #3 and #4.
I cannot think of an example where adding == true adds to the readability

Furthermore I’m not advocating that this rule gets destroyed I’m, just stating let’s not catch (Method() == false)

Here is my actual example that causes the code error for me

Example #7 suggested by sonarcloud
if (!log.Time.HasValue)

Example #8 not complient code which i assert is better
if (log.Time.HasValue == false)

we get another set of proses:

#7 if not log Time has a value
#8 if log Time has a value is false.

so #5 #6 is fine because when one reads not is right before valid the meaning is clear.

however in #7 #8 that does not remain true.
in #7 converted to prose, the not comes to early thus appearing to modify log or the log time instead of prase has a value.

I assert that if #7

if not log Time has a value

is more difficult to reason about then. #8

if log Time has a value is false

and as such we should consider not telling people the suggested solution is better in these cases.

Hello Luke,

Would you change your condition from this
if(Valid == false) {...}
to this
if(Valid == true) {...}
to keep your coding style consistent in case you need to swap the condition? I believe not.

We can surely find better or worse English-like names in favor of one or another approach. And we can both agree that there’s no conclusive answer to this using this metrics.

My point is that C# is not driven by English language. And we expect some knowledge of C# from C#-code readers. Operator ‘!’ is supposed to be well known and well understood.

Rule S1125 is about removing unnecessary Boolean literals and rule does that. You can deactivate the rule, if you consider it too noisy for your code base.

Would you change your condition from this
if(Valid == false) {...}
to this
if(Valid == true) {...}

No, the correct inverse of
if(IsValid == False ) {...} or if(!IsValid ) {...}
is
if(IsValid) {...}
where coding style consistent is in conflict with readablilty. Readability should always win.

We can surely find better or worse English-like names in favor of one or another approach. And we can both agree that there’s no conclusive answer to this using these metrics.

Right, so in situations where the ! operation is more difficult to reason about we should allow if(IsValid == false ) {...} without telling developers their code has a major issue.

My point is that C# is not driven by English language. And we expect some knowledge of C# from C#-code readers. Operator ‘!’ is supposed to be well known and well understood.

I agree that ! should be well know, but does not mean that it is always the best tool for the job. The only argument I can make for ! is that it is shorter and that is not enough of a reason to assume that it is always better.

Visual Studio itself makes this distinction correctly. if (thing == true) { }
comes with warning message. and reduces the opasity of == true
but it leaves ==false
true

I’m looking for a solution to be implemented with a scalpel, not a cleaver.

I still assert that == true is always redundant. and should be caught.

however both if(log.Time.HasValue == false ) {...} and if(!log.Time.HasValue) {...} are both valid with each having clear pluses and minuses. Sonar Could make this distinction like other code quality tools and not automatically say if(!log.Time.HasValue) {...} is good and the if(log.Time.HasValue == false ) is bad.

Rule S1125 is about removing unnecessary Boolean literals and rule does that.

I argue that it does that correctly for the == true example
as
if(Valid == true) {...} is the same as if(Valid) {...}
so simple it is removing a unnecessary Boolean literals

however
if(Valid == false) {...} is the NOT same as if(Valid) {...}
you are not just removing == false' you must and a !` to have it remain functionally the same.

So for the “false” example Sonar Could is NOT removing unnecessary Boolean literals.
but showing a dictating a preference for a “!” operator instead of a Boolean literal.

and that is fine (though I would not personaly support it)but that should be a different rule.

so this brings me back to the original request. 

Action request
As Sonar Cloud user I would like the code matching the example of if (booleanMethod() == false) { /* ... */ } Not to be alearted on.

Hi @lukehammer, @Pavel_Mikula,

It is true that booleanMethod() == false is easy to read alone. However we should consider two additional aspects when looking at a whole project:

  • Code symmetry. When reading if (myvar && b) || (!myvar && c) I easily understand that !myvar is the opposite of myvar.
    If I instead read if (myvar && b) || (myvar == false && c) I have to stop for a second and see if myvar can have a different value than true and false, such as null for a nullable boolean.
  • Code consistency. Using two different ways to check the same thing, i.e. !myvar and myvar == false, creates an incoherence, even if a small one. This makes the whole project more difficult to read, even if each line is easy to understand on its own.

For these reasons I agree with @Pavel_Mikula and we will keep this rule as it is. We are of course open to any additional feedback.
Disabling the rule is a valid solution if you don’t agree with it.

Does it make sense?

Note that this rule doesn’t raise on nullable booleans. Checking if they are true or false makes perfect sense.

@Nicolas_Harraudeau

I think you understand my main point.

It is true that booleanMethod() == false is easy to read alone.

Thank you for taking the time to understand me.

I understand your points regarding Code Symmetry and Code consistency.
I’m not attempting to say that all cases of !myvar should be replaced with myvar == false

but I would argue readability over Code Symmetry and Code consistency
and if we can do all three at the same time let’s do it.

Now I need to pick at your arguments just a bit

Code symmetry luke pick apart gently.

I have to stop for a second and see if myvar can have a different value than true and false, such as null for a nullable boolean.

No, you don’t. Nullable bool is not able to be used in that way.

bool? nullBool = true;
if (nullBool)
{ }// this code will not even complie 

same for the && statements
this also will not compile

   bool? nullBool = true;
   var result = nullBool && (1 == 2);// this code will not even complie 

Code consistency luke pick apart gently.
Right so I could say for my code or my teams’ code ‘== false’ is the right way, and don’t use ‘!’

I know this is going to be a stretch with this example but something like this bit me in the past.

if (!ion){ // feed the lion}
my point? some times ‘!’ look like ‘l’ or ‘I’ especially for us without perfect eyesight

but if written like this it would prevent the same error
if (ion == false){ // skip getting the electron micoScope}

Also, a common best practice is to name bool variables with the word Is e.g

if (!IsValid) vs if (IsValid == false)

I have been doing quite a bit of research related to this and the community is split on which is better.

the community seems to be split but in general, preferring ! to == false.

however, the fact remains booleanMethod() == false is not redundant.

Saying it’s redundant suggests that it can be removed without changing its meaning.

booleanMethod() == false is NOT the same as booleanMethod().

My argument now is while there are goods reason to prefer !booleanMethod() over booleanMethod() == false

What I’m looking for
I want the option to write in the cases where myvar == false is more readable. Without code reporting that my code sucks.

There are at least some reasons to prefer booleanMethod() == false and we should not punish developers that make that design decision.

I’m ok if this triggers a warning or info. with some other meaningful message.

but the statement “that == false is redundant”

is a lie as it can’t be removed without changing the meaning.

It’s not just me saying this at least 53 other people find this destination useful.

quote from link above

q == false is NOT redundant, just more verbose than ! . OTOH, == true is redundant. – dan04 Feb 25 '12 at 23:44

Luke now understanding where we are at as a sonar could community

Now with all this said. I’ve been heard out I understand that while I’m not alone. I am in the minority opinion.

I’m glad I had the conversation here first. This is better than having done a pull request to have it fixed and have my work rejected at that time.

1 Like

Hi Luke,

Thank you for correcting me about nullable booleans :wink:
I’ll check the link you shared.

Cheers,