Cleaner way to do a null check in C#? [duplicate]

Question

This question already has an answer here:

• C# elegant way to check if a property's property is null 21 answers

Suppose, I have this interface,

interface IContact
{
    IAddress address { get; set; }
}

interface IAddress
{
    string city { get; set; }
}

class Person : IPerson
{
    public IContact contact { get; set; }
}

class test
{
    private test()
    {
        var person = new Person();
        if (person.contact.address.city != null)
        {
            //this will never work if contact is itself null?
        }
    }
}

Person.Contact.Address.City != null (This works to check if City is null or not.)

However, this check fails if Address or Contact or Person itself is null.

Currently, one solution I could think of was this:

if (Person != null && Person.Contact!=null && Person.Contact.Address!= null && Person.Contact.Address.City != null)

{ 
    // Do some stuff here..
}

Is there a cleaner way of doing this?

I really don't like the null check being done as (something == null). Instead, is there another nice way to do something like the something.IsNull() method?

Answer 1

Do it in a separate method like:

private test()
{
    var person = new Person();
    if (!IsNull(person))
    {
        // Proceed
              ........

Where your IsNull method is

public bool IsNull(Person person)
{
    if(Person != null && 
       Person.Contact != null && 
       Person.Contact.Address != null && 
       Person.Contact.Address.City != null)
          return false;
    return true;
}

Answer 2

One way to remove null checks in methods is to encapsulate their functionality elsewhere. One way to do this is through getters and setters. For instance, instead of doing this:

class Person : IPerson
{
    public IContact contact { get; set; }
}

Do this:

class Person : IPerson
{
    public IContact contact 
    { 
        get
        {
            // This initializes the property if it is null. 
            // That way, anytime you access the property "contact" in your code, 
            // it will check to see if it is null and initialize if needed.
            if(_contact == null)
            {
                _contact = new Contact();
            }
            return _contact;
        } 
        set
        {
            _contact = value;
        } 
    }
    private IContact _contact;
}

Then, whenever you call "person.contact", the code in the "get" method will run, thus initializing the value if it is null.

You could apply this exact same methodology to all of the properties that could be null across all of your types. The benefits to this approach are that it 1) prevents you from having to do null checks in-line and it 2) makes your code more readable and less prone to copy-paste errors.

It should be noted, however, that if you find yourself in a situation where you need to perform some action if one of the properties is null (i.e. does a Person with a null Contact actually mean something in your domain?), then this approach will be a hindrance rather than a help. However, if the properties in question should never be null, then this approach will give you a very clean way of representing that fact.

--jtlovetteiii

Answer 3

Your code may have bigger problems than needing to check for null references. As it stands, you are probably violating the Law of Demeter.

The Law of Demeter is one of those heuristics, like Don't Repeat Yourself, that helps you write easily maintainable code. It tells programmers not to access anything too far away from the immediate scope. For example, suppose I have this code:

public interface BusinessData {
  public decimal Money { get; set; }
}

public class BusinessCalculator : ICalculator {
  public BusinessData CalculateMoney() {
    // snip
  }
}

public BusinessController : IController {
  public void DoAnAction() {
    var businessDA = new BusinessCalculator().CalculateMoney();
    Console.WriteLine(businessDA.Money * 100d);
  }
}

The DoAnAction method violates the Law of Demeter. In one function, it accesses a BusinessCalcualtor, a BusinessData, and a decimal. This means that if any of the following changes are made, the line will have to be refactored:

• The return type of BusinessCalculator.CalculateMoney() changes.
• The type of BusinessData.Money changes

Considering the situation at had, these changes are rather likely to happen. If code like this is written throughout the codebase, making these changes could become very expensive. Besides that, it means that your BusinessController is coupled to both the BusinessCalculator and the BusinessData types.

One way to avoid this situation is rewritting the code like this:

public class BusinessCalculator : ICalculator {
  private BusinessData CalculateMoney() {
    // snip
  }
  public decimal CalculateCents() {
    return CalculateMoney().Money * 100d;
  }
}

public BusinessController : IController {
  public void DoAnAction() {
    Console.WriteLine(new BusinessCalculator().CalculateCents());
  }
}

Now, if you make either of the above changes, you only have to refactor one more piece of code, the BusinessCalculator.CalculateCents() method. You've also eliminated BusinessController's dependency on BusinessData.

---

Your code suffers from a similar issue:

interface IContact
{
    IAddress address { get; set; }
}

interface IAddress
{
    string city { get; set; }
}

class Person : IPerson
{
    public IContact contact { get; set; }
}

class Test {
  public void Main() {
    var contact = new Person().contact;
    var address = contact.address;
    var city = address.city;
    Console.WriteLine(city);
  }
}

If any of the following changes are made, you will need to refactor the main method I wrote or the null check you wrote:

• The type of IPerson.contact changes
• The type of IContact.address changes
• The type of IAddress.city changes

I think you should consider a deeper refactoring of your code than simply rewriting a null check.

---

That said, I think that there are times where following the Law of Demeter is inappropriate. (It is, after all, a heuristic, not a hard-and-fast rule, even though it's called a "law.")

In particular, I think that if:

1. You have some classes that represent records stored in the persistence layer of your program, AND
2. You are extremely confident that you will not need to refactor those classes in the future,

ignoring the Law of Demeter is acceptable when dealing specifically with those classes. This is because they represent the data your application works with, so reaching from one data object into another is a way of exploring the information in your program. In my example above, the coupling caused by violating the Law of Demeter was much more severe: I was reaching all the way from a controller near the top of my stack through a business logic calculator in the middle of the stack into a data class likely in the persistence layer.

I bring this potential exception to the Law of Demeter up because with names like Person, Contact, and Address, your classes look like they might be data-layer POCOs. If that's the case, and you are extremely confident that you will never need to refactor them in the future, you might be able to get away with ignoring the Law of Demeter in your specific situation.

Answer 4

The second question,

I really don't like the null check being done as (something == null). Instead, is there another nice way to do something like the something.IsNull() method?

could be solved using an extension method:

public static class Extensions
{
    public static bool IsNull<T>(this T source) where T : class
    {
        return source == null;
    }
}

Answer 5

Such a reference chain may occurre for example if you use an ORM tool, and want to keep your classes as pure as possible. In this scenario I think it cannot be avoided nicely.

I have the following extension method "family", which checks if the object on which it's called is null, and if not, returns one of it's requested properties, or executes some methods with it. This works of course only for reference types, that's why I have the corresponding generic constraint.

public static TRet NullOr<T, TRet>(this T obj, Func<T, TRet> getter) where T : class
{
    return obj != null ? getter(obj) : default(TRet);
}

public static void NullOrDo<T>(this T obj, Action<T> action) where T : class
{
    if (obj != null)
        action(obj);
}

These methods add almost no overhead compared to the manual solution (no reflection, no expression trees), and you can achieve a nicer syntax with them (IMO).

var city = person.NullOr(e => e.Contact).NullOr(e => e.Address).NullOr(e => e.City);
if (city != null)
    // do something...

Or with methods:

person.NullOrDo(p => p.GoToWork());

However, one could definetely argue about the length of code didn't change too much.

Answer 6

A totally different option (which I think is underused) is the null object pattern. It's hard to tell whether it makes sense in your particular situation, but it might be worth a try. In short, you will have a NullContact implementation, a NullAddress implementation and so on that you use instead of null. That way, you can get rid of most of the null checks, of course at the expense at some thought you have to put into the design of these implementations.

As Adam pointed out in his comment, this allows you to write

if (person.Contact.Address.City is NullCity)

in cases where it is really necessary. Of course, this only makes sense if city really is a non-trivial object...

Alternatively, the null object can be implemented as a singleton (e.g., look here for some practical instructions concerning the usage of the null object pattern and here for instructions concerning singletons in C#) which allows you to use classical comparison.

if (person.Contact.Address.City == NullCity.Instance)

Personally, I prefer this approach because I think it is easier to read for people not familiar with the pattern.