CODEWITHSUNDEEP
c#
Arseni Mourzenko
Arseni Mourzenko

Reputation: 52321

Why checking for null before comparing a value to a constant?

Sometimes, a value must be checked for equality with a constant. In such cases, I've always seen the code like this:

if (!string.IsNullOrEmpty(text))
{
    if (text == "Some text here")¹
    {
        // Do something here.
    }
}

In my case, I would rather write:

if ("Some text here".Equals(text))
{
    // Do something here.
}

After all, if text is null, Equals will return false, which is expected. The inversion of a constant and a variable feels strange, but is still understandable for a beginner, and avoids the NullReferenceException which would be thrown with text.Equals("Some text here").

Am I missing something?

Why all source code I've seen use the syntax from the first example, and never from the second one?

¹ In real code, it would rather be a constant or a readonly field. To shorten the examples, I put the strings inline.

Upvotes: 2

Views: 2065

Answers (5)

jason
jason

Reputation: 241661

In such cases, I've always seen the code like this:

You're right to think that's odd and unnecessary, because it is. It's a completely superfluous null or empty check. Frankly, I'd admonish code like that in a code review. 

if (text == "Some text here") {
    // Do something here.
}

is perfectly fine and that's what I'd use.

Am I missing something?

No, you're not missing anything.

Why all source code I've seen use the syntax from the first example, and never from the second one?

Because you're looking for love in all the wrong places?

Upvotes: 11

David Heffernan
David Heffernan

Reputation: 613063

Your version is fine so long as you have a literal or something that you know will not be null.

If, on the other hand, you had 

if (text1.Equals(text2))

then clearly you would need to defend against text1 being null.

But I would stop using Equals here and use == which removes the need for the null check

if (text1 == text2)

Are you preferring to use Equals because of your Java roots?

Upvotes: 2

Tim S.
Tim S.

Reputation: 56546

I would just use if (text == "Some text here"). It's clear, concise, and fast. The IsNullOrEmpty check you refer to might be a (most likely useless) micro-optimization.

Upvotes: 0

Bek Raupov
Bek Raupov

Reputation: 3777

I would think it is in human nature, when you compare smth, you always compare "your" stuff against someone else. not the other way around, i think same natual thinking went to C# coding as well :)

Upvotes: 0

Kendall Frey
Kendall Frey

Reputation: 44346

There is nothing wrong with just this.

if (text == "Some text here")
{
    // Do something here.
}

There is no need to check for null/empty, since it won't be equal anyway.

If you wish to use the Equals method, there is a version that isn't sensitive to null values.

if (string.Equals(text, "Some text here"))
{
    // Do something here.
}

Upvotes: 5

Related Questions