I don't like this... Is this cheating the language?

Question

I have seen something like the following a couple times... and I hate it. Is this basically 'cheating' the language? Or.. would you consider this to be 'ok' because the IsNullOrEmpty is evaluated first, all the time?

(We could argue whether or not a string should be NULL when it comes out of a function, but that isn't really the question.)

string someString;
someString = MagicFunction();

if (!string.IsNullOrEmpty(someString) && someString.Length > 3)
{
    // normal string, do whatever
}
else
{
   // On a NULL string, it drops to here, because first evaluation of IsNullOrEmpty fails
   // However, the Length function, if used by itself, would throw an exception.
}

EDIT: Thanks again to everyone for reminding me of this language fundamental. While I knew "why" it worked, I can't believe I didn't know/remember the name of the concept.

(In case anyone wants any background.. I came upon this while troubleshooting exceptions generated by NULL strings and .Length > x exceptions... in different places of the code. So when I saw the above code, in addition to everything else, my frustration took over from there.)

Answer 1

There is nothing wrong with this, as you just want to make certain you won't get a nullpointer exception.

I think it is reasonable to do.

With Extensions you can make it cleaner, but the basic concept would still be valid.

Answer 2

Theres nothing wrong with this.

if(conditions are evaluated from left to right so it's perfectly fine to stack them like this.

Answer 3

If you are asking if its ok to depend on the "short circuit" relational operators && and ||, then yes thats totally fine.

Answer 4

You're taking advantage of a language feature known as short circuiting. This is not cheating the language but in fact using a feature exactly how it was designed to be used.

Answer 5

This is valid code, in my opinion (although declaring a variable and assigning it on the next line is pretty annoying), but you should probably realize that you can enter the else-block also in the condition where the length of the string is < 3.

Answer 6

This is perfectly valid and there is nothing wrong with using it that way. If you are following documented behaviour for the language than all is well. In C# the syntax you are using are the conditional logic operators and thier docemented bahviour can be found on MSDN

For me it's the same as when you do not use parenthesis for when doing multiplication and addition in the same statement because the language documents that the multiplication operations will get carried out first.

Answer 7

I find it OK :) You're just making sure that you don't access a NULL variable. Actually, I always do such checking before doing any operation on my variable (also, when indexing collections and so) - it's safer, a best practice, that's all ..

Answer 8

This code is totally valid, but I like to use the Null Coalesce Operator for avoid null type checks.

string someString = MagicFunction() ?? string.Empty;
if (someString.Length > 3)
{
    // normal string, do whatever
}
else
{
   // NULL strings will be converted to Length = 0 and will end up here.
}

Answer 9

That looks to me like a perfectly reasonable use of logical short-circuitting--if anything, it's cheating with the language. I've only recently come from VB6 which didn't ever short-circuit, and that really annoyed me.

One problem to watch out for is that you might need to test for Null again in that else clause, since--as written--you're winding up there with both Null strings and length-less-than-three strings.

Answer 10

It makes sense because C# by default short circuits the conditions, so I think it's fine to use that to your advantage. In VB there may be some issues if the developer uses AND instead of ANDALSO.

Answer 11

I don't think it's any different than something like this:

INT* pNumber = GetAddressOfNumber();

if ((pNUmber != NULL) && (*pNumber > 0))
{
  // valid number, do whatever
}
else
{
  // On a null pointer, it drops to here, because (pNumber != NULL) fails
  // However, (*pNumber > 0), if used by itself, would throw and exception when dereferencing NULL
}

It's just taking advantage of a feature in the language. This kind of idiom has been in common use, I think, since C started executing Boolean expressions in this manner (or whatever language did it first).)

Answer 12

Thanks for all the quick responses... this is the guidance I was looking for.

...but I will still say I don't like how it reads, even if short-circuiting is a useful feature of the language ;)

Answer 13

If it were code in c that you compiled into assembly, not only is short-circuiting the right behavior, it's faster. In machine langauge the parts of the if statement are evaluated one after another. Not short-circuiting is slower.

Answer 14

In C++ it was so that the order in which condition checks are performed is not guaranteed. Is it different in C#?

EDIT: So what? I'm being downvoted because I admitted I didn't know something? Is it considered shameful or what? It is rather a shame to have such untolerant jerks in SO community.

Answer 15

Writing code cost a lot of $ to a company. But maintaining it cost more !

So, I'm OK with your point : chance are that this line of code will not be understood immediatly by the guy who will have to read it and correct it in 2 years.

Of course, he will be asked to correct a critical production bug. He will search here and there and may not notice this.

We should always code for the next guy and he may be less clever that we are. To me, this is the only thing to remember.

And this implies that we use evident language features and avoid the others.

All the best, Sylvain.

Answer 16

Relying on short-circuiting is the "right thing" to do in most cases. It leads to terser code with fewer moving parts. Which generally means easier to maintain. This is especially true in C and C++.

I would seriously reconsider hiring someone who is not familiar with (and does not know how to use) short-circuiting operations.

Answer 17

A bit off topic but if you rand the same example in vb.net like this

dim someString as string
someString = MagicFunction()
if not string.IsNullOrEmpty(someString) and someString.Length > 3 then
    ' normal string, do whatever
else
    ' do someting else
end if

this would go bang on a null (nothing) string but in VB.Net you code it as follows do do the same in C#

dim someString as string
someString = MagicFunction()
if not string.IsNullOrEmpty(someString) andalso someString.Length > 3 then
    ' normal string, do whatever
else
    ' do someting else
end if

adding the andalso make it behave the same way, also it reads better. as someone who does both vb and c' development the second vb one show that the login is slighty different and therefor easyer to explain to someone that there is a differeance etc.

Drux