Boolean literals should not be redundant C# csharpsquid : S1125

SonarCloud
1

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.

2 Likes
2

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.

3

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.

4

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.

5

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.

7

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.

8

@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
9

Hi Luke,

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

Cheers,

10

I’ve asked for this before as well, for different reasons. I’ve had the pleasure of experiencing more than one bug because a team member missed the negation operator in an if (!IsValid()) {.... This was ~10 years ago when I was more or less “the kid” on a team with an average age north of 45. My eyesight has always been bad, but now that I’m only a few years from that average age myself, I really appreciate it when the negation operator isnt used sandwiched in between two other mostly upright line characters.

This particular rule irritates because its not consistent in its logic for the one case and the only defense is one of subjectivity and non-empathetic “we’ve always done it this way” bias.

That one case also presents a legal problem under US Federal law as it singles out and targets at least two of the protected classes to make work unequally difficult for them- Age, because vision decreases as we get older, and Disability - those with vision problems.

This rule needs to be corrected to not complain about == false, or be given an option to disable that specific discriminatory part of it.

11

I think I see your arguments, and why you’d like it to be prose - I don’t always agree that is optimal though. Training yourself to read binary arguments in code will help you a lot in more complex scenarios, so I don’t see the same value in it as you do. I much prefer consistency and symmetry because my pattern recognition kicks in, and I understand it right away. I don’t need prose structure for that.

With that said, I also believe “== false” will slow down my reading of the code, and came here exactly because of such a case:

When “== false” is used, I’m assuming there’s a reason to it, because it’s a comparison that is done before handing it over to the “if” statement, to therefore change the output. And then nullable comes into the picture again. Using “== false” is often used to ensure exactly false and not null, and when I see code with “== false” I’m expecting the type to by nullable. Thus, now I have to go see if that is correct, because what would null then mean for this scenario? If “!myVar” was used, I’d know it is a regular boolean, or else the code doesn’t compile. Thus my understanding of the code increases through using the not operator over using a comparison with false.

But I don’t like testing for negative cases to begin with - so I tend to go with true statements instead, and construct the code in a way that there’s as few modifiers to my booleans as possible. So I’d often just invert the body of if/else so the if part contains the simple “if(myVar)” because I find that to have the highest readability.

12

I following your thoughts

And if we want to catch this with some other rule with some other justification that’s a meaningful conversation to be had.

I’m not trying to defend that the style (booleanMethod() == false) his best in all cases. Your counterexamples are good. I’m simply suggesting that there is at least one case where (booleanMethod() == false) is better than (!booleanMethod()) and developers should be able to make this decision in the context of their own application. but this whole argument is really not the point I’m trying to make.

My main point is if (booleanMethod() == false) { /* ... */ } simply not redundant

I agree that if (booleanMethod() == true) { /* ... */ } is redudent you can remove the == true and I think it improves the readability and should be done and is corrected to be pointed out.

you can’t do that with if (booleanMethod() == false) { /* ... */ } removing == false changes the operation.

Again we can have meaningful discussions about why we think it is or is not a best practice. And whether we should catch it all. but catching it with this rule and saying that it is redundant is simply a lie.

So I guess my question back to the community really is.

Are we OK with this little lie?

Is this worth doing right?

Is it ok to lie if the lie achieves the goals that were desire?

13

“prose” - i dont get what you mean by this term. I want something that isnt redundant to stop being called redundant. true == true is redundant, true == false is not redundant.

!true and false mean the same thing but they wont ever be redundant because a condition is always looking for true in order to continue with that conditional branch.

The style is important to me also because I have had to deal with the defects that something like if (!IsValuable(thing)) creates, but that would be taken care of if this rule did what it is supposed to do and not try to force someone else’s style.

If you are reading == false as having different meanings or connotations from !, those assumptions are yours.

Nullables should be checked for a value prior to inspecting the actual value. Thats kind of the point of having the null, its an extra state for “This has not been set”. We had to carry around a separate boolean to represent being set or not.

bool? myBool
if (myBool.HasValue == false ) {... //not redundant
if (myBool.HasValue == true && myBool.Value == true) {... // both parts redundant with the == true

I agree that “true” evaluations are the better way, but we have things like “Is my string null or whitespace?” where the actual question is something like “can we continue to use this string value” or “should an error be thrown”, But the incorrect part of this rule isnt about the redundancy of true == true. Its that true isnt redundant with false in any way you choose to describe false

14

@lukehammer - I saw that you responded after i had typed up that last message. Sorry for the duplication =D

To answer your questions…

Are we OK with this little lie?

I dont know if I’d call it a lie. Its logically incorrect, which in this profession is probably going to be more offensive than someone being untruthful. The longer it stays logically incorrect, the more offensive it becomes.

Is this worth doing right?

Someone went out of their way to code this extra style scenario. It was worth doing wrong, so it must be worth doing right.

Is it ok to lie if the lie achieves the goals that were desire?

no, because doing this is flatly unethical, professionally speaking. Some workplaces have zero tolerance policies regarding this and any unfair competition (internal or external), even when it brings a significant benefit to the company.

15

You, being so sure about that part, had me second-guessing myself, so I guess I was not as sure about the matter as I thought. It turns out I was right though. Nullable only has operator overload for equality comparer and not for the “!” prefix. So from a C# perspective, those two statements are not the same.

That is related to the first post, which might also have thrown me off track:

That’s why I started to argue for the implementation of this rule - because, to me, the code is more readable without the == false options in the code, because I can not know, just from looking at the code, if there is 1 or 2 other values (true and perhaps null), which affects the code further down.

But I agree with you both, that the use of the word “redundant” is, at best, misleading, and at worse downright false.

If you look at the original spec that this origins from, it says:

[RSPEC-1125] Boolean literals should not be redundant - SonarSource

And I think the intention is simply to make the result more concise. Using ! instead of == false accomplishes that, but saying that == false is redundant is incorrect (as you so correctly pointed out, before this page also became a discussion about style/readability).

So my personal preference would be to:

  1. Change [RSPEC-1125] both in wording and implementation to only focus on redundancy, so the == false option is no longer non-compliant.
  2. Add a new spec for “boolean expression should be simplified” (I’d argue the use of “should” is correct, because if you think it should not, you should not have the rule active), and that it is active by default - to keep the current implementation for all, make all the wordings correct, and let people choose to disable the rule to be able to keep == false in their code base an still be compliant with their active rules of SQ.
16

Hi @lukehammer

Seems to me that this rule is dividing the community. It relies on personal preferences and some like it and some just don`t. Your suggestion of splitting the rule is nice, but at this moment I am not sure it is worth investing time to split it.

17

@Caba_Sagi Word meaning is not a matter of personal preference. We can consult a dictionary and none of them will consider (true,false) redundant.

There are enough other reasons given I would think that sonar ought to remove the coerced style part of this rule and extricate itself from the conflict. The least of which being that quality is not a popularity contest, and the coerced style is easier to make a mistake with when reading.

EDIT:
Also the readability of boolean expressions (like (!IsReady)) have been recognized as problematic enough that additional pattern matching features have been added to the language. Hopefully this sonar rule does not start flagging (IsReady is false) as redundant also and wanting to change it too.

The specific decision making logic for this also seems to conflict with S4058, which encourages redundancy with string comparisons rather than just allow usage of the default, because it…

makes your code clearer and easier to maintain

Please address the inconsistency.

@JohnyWS - .HasValue is a boolean and doesnt have any operator overloading, so I dont know what you mean.

19

Hi @StingyJack

This rule is implemented for more than 10 different languages and this implementation is consistent with the other languages as well. So, currently we do not plan to invest the effort to split this rule. Thank you for your understanding.

20

I understand that you have a bug in 10 different languages and arent investing in fixing it. If that doesn’t frame the problem well enough for you then I dont know what will. w/e - as valuable as this rule could be its not getting enabled because this additional inconsistent behavior can contributes to bugs.