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.)

Raceway

There's a reason these type of operators are called "conditional operators" - from MSDN: "The conditional-AND operator (&&) performs a logical-AND of its bool operands, but only evaluates its second operand if necessary." msdn.microsoft.com/en-us/library/2a723cdk(VS.80).aspx – Gerstein

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

maybe this is a chance for the [facepalm] taxonomy badge :P – Nicotinism

I don't really understand why this has been downvoted as it's a reasonable question. Some people just don't realise these things, let them bloody learn god damned elitists! – Guendolen

This is one of the reasons they are working on getting us the NonNullable contracts seen with Spec# – Boardwalk

Answer 1

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.

Choong

-1: writing softare is a thing, maintaining them is an other story. When your boss ask you to correct existing code in the minute, this is the kind of thing you can easily overcome. What about 2 ifs ? Or a comment saying what is happening here ? People who maintain softwares are much less clever than people that write them (I know it, I was maintaining softwares for a while). The problem is : code writers think that everybody are as clever as they are. But this is not true. When you write code, please, think to us ! We have to read a lot of code and react in the minute, and we are not clever ! – Pulverable

@Sylvain, agreed clever code is harder to maintain. However this code is not attempting to be clever. It's using a well known language feature in the way it was intended to be use. Also this is not a just C# feature, it's a feature of pretty much any language (C++,F#, VB.Net, C#, C, etc ...). It is completely reasonable to expect other programmers to understand the intended function of this code – Choong

+1 - This is an extremely standard practice in C, C++, C#, Perl, Python, Ruby, PHP... – Do

@Sylvain: If you don't understand the fundamentals of your professional tools (and a short-circuiting boolean operator is a fundamental), then you should not be touching production code. </petpeeve> – Superscribe

@Sylvain, like the others said, this is fundamental. This would be like not allowing x=Function1(Function2()) or x=(y+1)*3 because it's 'not obvious'. There's some things all programmers should know and short circuiting is one of them. – Parados

Short circuit as soon and as much as possible (as long as the code doesn't end up looking like gibberish). &&, ||, return, continue, break, and exit are your friends. – Petrinapetrine

Answer 2

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

Slab

Not only is it fine, but I think most people would consider it a language feature – Ching

always code considering that it may short circuit, so it will run faster. – Berkin

@KM - almost always but not if the second operand is a method that needs to be executed in every case. Of course, one would question the merits of a method such as this being executed within the condition. Better to execute it beforehand and hold the result in a variable. That said, I have seen this happen and known of people head-scratching whilst trying to work out why the method was not being executed as expected. – Blueprint

I don't know about C#, but in Java there's also non-short-circut relational operators. They are the same except they consist of one character instead of two, | and &. These always execute the second operand no matter what... (well as long as the first operand doesn't throw an exception). – Slab

@MK Are you sure those aren't bitwise operators? If you;re using those to check logical conditions you're gonna have some bugs in your code! You can;t get away with that mistake in C# because it is very strongly typed, using a bitwise operator will give a conversion error at compile time. – Yumuk

@TL They are both bitwise and relational operators, it depends on the context. If the operands are booleans then you get a non-short-circuit relational operator. If the operands are integral type you get a bitwise operator. Again I'm just talking about Java here, I'm not a C# programmer. Its in the Java spec here: java.sun.com/docs/books/jls/third_edition/html/… – Slab

Answer 3

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.

Wingfooted

Can you post the "with extensions" sample? – Bethsaida

Just have an extensions: public static bool IsNullOrEmpty(this string str) { return string.isNullOrEmpty(str); } Then you can just all if (!someString.IsNullOrEmpty() && someString.Length > 3) { ... } I think it looks cleaner, but is basically the same. – Wingfooted

Answer 4

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.
}

Lashawna

However, that changes the value of the variable, which is not always desirable. – Pederson

Answer 5

Theres nothing wrong with this.

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

Furr

Answer 6

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.

Ching

Answer 7

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.

Bewhiskered

About the operator precedence thing: Parentheses are not just for grouping operations, they can improve the readability of an expression too if used properly. – Gerstein

Answer 8

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.

Douche

VB6 didn't have short-circuit operators? Wow I've heard people joke about it being brain-dead but that is a really shockingly stupid thing to leave out of a language... – Noenoel

Nope. And VB.Net is still half-crippled in this respect, because And and Or still don't short-circuit. You have to use AndAlso and OrElse to get short-circuiting. – Douche

I did 7 years VB programming, switched to C# (been there for 7 years now) and got caught up on this no-short-circuit thing just the other day when having to maintain a VB macro. The no-short-circuiting thing has been left in there for back-compat because some people would write if statements with side effects (where the last "test" in the statement would always run). How mental is that? – Terpstra

Figured it was back-compatibility (though so much else fundamentally changed that it seems a silly thing to preserve), but I can still mourn. If only the dependency on side effects would bit the idi...um, challenged persons who wrote them instead of we who come after.... – Douche

Answer 9

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.

Durga

Answer 10

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 ..

Hiramhirasuna

Answer 11

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.

Grearson

True. While I'm really glad they added AndAlso, and now always use it, I wish they'd just fixed the behavior of And (same for Or/OrElse). – Douche

Answer 12

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).)

Hyetal

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.

Tagmemics

Answer 14

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.

Pulverable

I code on the assumption that the maintainer will be less intelligent and less experienced than I am. I do, however, assume that the maintainer will not be confused by common, standard, language constructs. If you have any problem whatsoever with short-circuit operators, you have no business touching a critical bug. – Jacquesjacquet

Agreed strongly with David here. The "if (foo() && bar())" construct is so standard that it's actually more clear and easier to read to a moderately experienced programmer. – Do

You should be right, but since 2000, a lot of non computer scientists poeple were hired to create and maintain important softwares (in a previous assignement, C functions having more than 4000 loc + 9 levels of indentation was common). These guys may be as clever as anybody, but a lot of them never read a book about C before we ask them to maintain C code. Worst, when a bug is found, they often have a lot of pressure to correct it NOW - they don't have the time to fiddle around - code must be crystal clear. Anyway, short-circuiting is ok as long as it is not used randomly here and there. – Pulverable

@Sylvain: I don't write code for people who don't know how to program. Especially if they don't even have enough professional drive to pick up a book about C on their own time when their responsibilities morph in that direction. – Superscribe

@Greg D. You're not the only one ! I currently trying to understand what some superior IQ does with a lot of design patterns in order to... save simple data. 1 zillons sources, a hight inheritance tree, no comment at all, wow ! What a male attitude ! Beside, correcting a simple bug take 2 or 3 days. Were they professionals ? Your call... – Pulverable

Answer 15

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

Expanded