Concise null checking vs readability

Question

I've got a case where I need to call a couple of methods, checking if they return null, and return some property from the result of the second when neither returns null.

I've thought of two ways to do this - one "verbose" and one "concise". For the purposes of this question, I'll just call the return types 'dynamic' since I'm not interested in those details here.

private dynamic VerboseVersion()
{
     var a = CallMethodA();
     if ( a == null ) 
     { 
         return null; 
     }

     var b = CallMethodB( a );
     if ( b == null ) 
     { 
         return null; 
     }

     return b.SomeProperty;
}

private dynamic ConciseVersion()
{
    dynamic a;
    dynamic b;

    if ( ( a = CallMethodA() ) == null ||
         ( b = CallMethodB( a ) ) == null )
    {
        return null;
    }

    return b.SomeProperty;
}

I the first because the intent at each step is very obvious but I find it somewhat verbose; I like the second because the intent of the function seems more obvious and it is less verbose but it may be a bit harder to follow at first.

Thoughts?

Update: Corrected a typo and corrected the formatting of the "Verbose" version to what I would actually use in committed code.

Answer 1

I take great pleasure in the fact that your "concise" version is longer than your "verbose" version!

If you believe one version to be more readable and the other to be more obscure, I would argue going with readability over obscurity. One obscure detail is that you're relying on people understanding

(a = CallMethodA()) == null

Some people might think that to be an error, not realizing the expression on the left actually has a value.

If you want to use less lines of code, you could go with something like this

var a = CallMethodA();
var b = (a == null) ? null : CallMethodB(a);
return (b == null) ? null : b.SomeProperty;

Which isn't altogether different than your original if/else chain, but is a bit more compressed and has just the one return statement.

Answer 2

I would always prefer the verbose one. It is clear, readable and most importantly it not easy to misunderstand it. I never try to make the code shorter, in fact I believe that even if the code is longer total cost of it may be lower.

Because a misunderstanding or error because of ambiguity always costs way more than typing a few more lines.

Answer 3

I also agree with @oberfreak that I feel like that in your example you are using null as an error value to manage errors and do some protections, have you considered using exceptions?

In many cases to avoid uncatched exceptions and try-catch blocks many of us use the null as an error value and probably use another kind of aproximation would be a good alternative.

Also, another option I would suggest is to call CallMethodA inside CallMethodB and there control the return value of CallMethodA

Answer 4

I would modify CallMethodB to return null when it is passed null and then use the following:

private dynamic VerboseVersion()
{
    var b = CallMethodB( CallMethodA() );
    if ( b == null )
    {
        return null;
    }
    else
    {
       return b.SomeProperty;
    }
}

I prefer to avoid assignments in conditions.

Answer 5

These two versions do not have the same semantics. The first version guarantees CallMethodB receives a non-null param "a" whereas the second does not. I don't know what CallMethodB requires, but you could get null pointer exception in b. I personally prefer the first version, clear and precise, I don't feel you lose any readability. If CallMethodB accepts null param, then you might be able to simplify the code by just:

b = CallMethodB(CallMethodA());

Correction: As Anthony pointed out, the "||" at the end actually would short circuit the evaluation when a is null, so there is no semantic difference with these two version. I've misread the original post. Sorry for my jump the gnu coclusion. Though, I still prefer version 1, it feels just less confusion.

Answer 6

I am totally agree with Serkan Özkan.

Despite the fact that the ConciseVersion looks much more "smarter" I wouldn't sacrifice redability and would vote for VerboseVersion. This is something most of the developers used to. Moreover, I would put return statements in a separate line, so it would be clear that if something is null then null should be returned

Answer 7

Another variant is to create an extension method like:

    public static class GeneralExtensions
    {
       public static dynamic GetNext<T>(this T objectToProcess,
                                        Func<T, dynamic> getNextCallback) 
           where T : class
       {
          if (objectToProcess != null)
          {
             return getNextCallback(objectToProcess);
          }
          else
          {
             return null;
          }
      }
  }

Then your code will be:

return CallMethodA()
       .GetNext(a => CallMethodB(a)
       .GetNext(b => b.SomeProperty));

Answer 8

I think the concise version is longer and more confusing, and emphasizes the wrong return (assuming the value of SomeProperty is what is really of interest).

For the verbose, I don't like returning the same value more than once, even if it is a constant - so a variation on the verbose:

private dynamic MyVerboseVersion() 
{
   var a = CallMethodA();
   if (a != null) {
       var b = CallMethodB(a);
       if (b != null) {
           return b.SomeProperty;
       }
    }

    return null;
}

I don't really like the burgeoning arrow or the fall through else, but it is how I often solve this pattern.

Answer 9

If I have the opportunity to modify methods, and expect that the result of a method would return a null, I will rewrite and follow the TryXXX pattern.

private dynamic TryVersion()
{
    dynamic a;
    dynamic b;

    if (TryCallMethodA(out a)) {
        if (TryCallMethodB( a, out b )){
            return b.SomeProperty;
        }
    }

    return null;
}

If you really want to shorten this, you could:

if (TryCallMethodA(out a) && TryCallMethodB( a, out b )){
        return b.SomeProperty;
}  

Note: the second snippet is untested but I believe it would work as the left TryCall must be resolved to move on to the right TryCall.

Answer 10

Why does the Methods CallMethodA and CallMethodB return null? Wouldn't it be better to throw an exception, that you can signal that something went terribly wrong?
In my opinion returning null is always bad for developers who are using your code, becaus they don't know what went wrong and what to change.
At your example you are expecting CallMethodB to return an Object. So if nobody throws an exception everything has just worked fine, use the return value, if it is null let the nullReferenceException occur and enjoy writing well code.
Removing Null checks would increase your readability and the reusablility of your code!

SampleObject CallMethodA(){
      if(Some())
         return new A();
      throw new SomeThingTerribleException(Can't create SampleObject due to lack of Some");
 }

SampleObject CallMethodB(SampleObject obj){
      if(obj.IsSomethingSet())
         return new B(obj);
      throw new SomeThingTerribleException("Can't create SampleObject due to lack of Some");
 }
void Main(){
    var a = CallMethodA();
    var b = CallMethodB( a );
    return b.SomeProperty;
}

As you might see no null, no dirty null-checks just clear exceptions for the user of your Methods.