How is the first example of compliant more error prone for S3256?

SonarCloud
,
1

The SonarSource Code Analyzers Rules Explorer shows these as compliant but makes a comment about the first one…

name != null && name.Length > 0 // Compliant but more error prone
!string.IsNullOrEmpty(name)
string.IsNullOrEmpty(name)

How is the first one more error prone, and why doesnt this suggest uising isNullOrWhitespace?

2

Hey Andrew

When you see IsNullOrEmpty it’s immediately clear what is being checked – while the more verbose option (used before isNullOrEmpty was introduced) requires more thinking, and is also easier to make a typo (name = null, !name.Length > 0, name.length < 0). This isn’t about how reliable the code is – but how likely it is that a developer will make a mistake.

string foo = "";
string bar = "     ";

While IsNullOrEmpty and IsNullOrWhitespace is equivalent for foo, it isn’t for bar. Stated another way, the length of a string includes whitespace, so isNullOrWhitespace won’t be checking for name.Length > 0.

3

I totally understand and agree with this, but…

  1. most of the time developers actually mean “is null or is only whitespace”, even when the code does not have a .Trim() before the length
  2. this viewpoint is counter to the logic given for other sonar rules.

For the first one I was just thinking that the rule could also suggest the whitespace function if that was the possible intention.
The second one is a bucket worms thats already open in other threads I started or commented in. I’m not going to rehash it here.PM me if you cant find the links and I’ll send them to you.

4

Hey Andrew.

I am not responsible for defining rules in our language analyzer, but reading your response maybe there’s room for another rule that checks return String.IsNullOrEmpty(value) || value.Trim().Length == 0; or value != null && valueTrim().Length > 0 and suggests using isNullOrWhitespace instead – what do you think? It’s even well documented to have superior performance

It’s worthy of a suggestion at least.

If you really think that most of the time developers intend to use isNullOrWhitespace when they use isNullOrEmpty (checking our own analyzer, we use both), that seems like a separate suggestion you’ll probably have to debate a bit more thoroughly with out C# experts.

Maybe we can just start fresh, as I know some past threads have gotten a little heated (no blame assigned in either direction). Can you give an example of a rule with contrary logic?

5

Either a second rule or suggestion in the existing rule would be great.

For each of the uses of the IsNullOrEmpty(), all you have to ask is “would the program behave as expected if \0\0\0\0 (null byte string) or \t\r\n (tab, carriage return. line feed) were supplied?”

Most of the time, we would expect the program to behave as if it were given string.Empty or null. But that wont happen with IsNullOrEmpty()

I dont want to debate a c#/.net topic without c# experts.

Its only heated for me when someone close-locks a thread or makes me a victim of policy because silencing someone or adding grief when it could have been avoided are not the way to treat others, online or IRL

I’ll collect the examples and work out the phrasing (so that directness and concision are not mistaken for someone’s perception of my feelings at the time of writing) over the weekend.