views:

1267

answers:

4

I have a question on programming style and C# language design in general, I'd love to know if there is a better way to do what I'm doing.

If you have a complex data object, with properties that can be null but you want to check or operate on data if it is there, you cannot write a line like so

if(Myobject.MyNestedObject != null || Myobject.MyNestedObject.Property != null)
{
   //code
}

Because the compiler will actually call both lines of code to evaluate the if statement.

Instead you must (I believe) write :

if(Myobject.MyNestedObject != null)
{
   if(Myobject.MyNestedObject.Property != null)
   {
      //code
   }
}

Is there a better style than this? I'm trying to think of how to use null coalesce (??) but it would still throw if you try to use anything of MyNestedObject in the same statement.

More info:

    L_01b4: ldarg.1 
    L_01b5: callvirt instance class [Myassembly]MyClass.MyObject [MyAssembly]MyClass::get_MyObject()
    L_01ba: brtrue.s L_01cc
    L_01bc: ldarg.1 
    L_01bd: callvirt instance class [MyAssembly]MyClass.MyObject [MyAssembly]MyClass::get_MyObject()
    L_01c2: callvirt instance class [MyAssembly]MyClass.MyNestedObject [MyAssembly]MyClass.MyNestedObject::get_MyNestedObject()
    L_01c7: ldnull 
    L_01c8: ceq 
    L_01ca: br.s L_01cd
    L_01cc: ldc.i4.0 
    L_01cd: stloc.2 
    L_01ce: ldloc.2 
    L_01cf: brtrue L_0285
    L_01d4: nop

From my understanding it's saying that at L_01ba if the call returns true, not null or non-0 (i.e if the object is null, the branch isn't taken and then control flow continues linearly). This then will of course execute L_01c2 which will throw a null reference exception, as Myclass.MyObject is null.

Have I missed something. This is the .net 3.5 C# compiler.

+2  A: 

C# uses lazy checking, so your first code should be fine (with || changed to && of course!)

Update - Here it is: http://msdn.microsoft.com/en-gb/library/6373h346.aspx " The operation

x || y

corresponds to the operation

x | y

except that if x is true, y is not evaluated (because the result of the OR operation is true no matter what the value of y might be). This is known as "short-circuit" evaluation. "

Update again - should be using &&!

Chris
I believe the term is 'short curcuiting'
Jay Riggs
Matt Hamilton
I'm not denying what you're reference has said at all, but I've used || and it threw a null reference exception on the line?? Perhaps C# determined that it had to check both?
Spence
@Spence: are you sure MyObject is != null in your example? I double checked and the || is using lazy checking. The statement: if (string1 == null || string1.Length == 0), works fine if string1 is null. If the checks are reversed it fails with a NullReference exception.
sipwiz
I've pasted the IL into my question.
Spence
+4  A: 
if( Myobject.MyNestedObject != null && 
             Myobject.MyNestedObject.Property != null)
{
//code
}
aJ
Wont compile - "Cannot implicitly convert type 'xxxx' to 'bool'"
Chris
Even if this compiled in C#, it's not as readable, even if it is more succinct.
Scott Ferguson
aJ
+14  A: 

Combining @Chris and @aJ answer:

I think you want the && operator, not ||.

if (Myobject.MyNestedObject != null &&
    Myobject.MyNestedObject.Property != null)
{
    //code
}

And C#'s && operator use short-circuit evaluation, so if the first expression returns false, the second expression will not be evaluated.

...

chakrit
A good nights rest makes what should have been bleedingly obvious. By writing the two lines it's effectively performing what the and operation does.
Spence
+1  A: 

I'm going to add the obligatory advice that having to dig through layers of public properties usually means you are exposing too much internal state, and that the classes you are traversing should be doing this work for you. I would also expect an object to ensure its properties do not return null in the first place.

There are edge cases of course, but these are good rules of thumb.

Jim Arnold
your perfectly correct, but I have to consume XML from a legacy source. As such the data I get will always be incomplete and this code is in the parser between raw data and a usable object. As such I have to check nulls, everywhere, which is why I wanted to know if there was a better pattern.
Spence