Optimize for the Happy Path: Null Checks

Introduction

It’s what we’ve been taught since we’ve started programming. Don’t trust user input. Validate everything. Think of, and handle, every possible type of exception that can be thrown from a given operation. Oh, make sure you log everything too.

But sometimes, advice can be taken, then implemented the wrong way. Defensive programming is not analogous with null checks. Null checks everywhere. Null checks for no reason.

This post will walk through an MVC application and talk about how to use null checks effectively. Un-needed null checks can obfuscate business intent, act as a barrier to new contributors, and lead to a LOT of unnecessary code being written.

An Example of Null Checks Obfuscating Intent

I was recently doing a code review in one of the open source projects I contribute to and I came across this code in a Notification handler. This handler is responsible for generating an email message and subject to send:

var taskSignup = await _context.TaskSignups  
    .Include(ts => ts.Task).ThenInclude(t => t.Event).ThenInclude(a => a.Organizer)
    .Include(ts => ts.Task).ThenInclude(t => t.Event)
        .ThenInclude(a => a.Campaign).ThenInclude(c => c.CampaignContacts).ThenInclude(cc => cc.Contact)
    .Include(ts => ts.User)
    .SingleAsync(ts => ts.Id == notification.SignupId);

var volunteer = taskSignup.User;

...

var subject = volunteer.FirstName != null && volunteer.LastName != null  
    ? $"{volunteer.FirstName} {volunteer.LastName}" : volunteer.Email;

When this handler builds the subject variable, there are null checks being performed. If we're missing the volunteer's first and last name, we use their email address. If you were to look at this code as a new programmer to the project, your assumption would be that the volunteer's FirstName and LastName are allowed to be null.

Instead of taking my assumption (informed by the code) at face value, I started backtracking in the code to see:

  • what a volunteer represents (what IS a volunteer anyhow?)
  • why a volunteer would be allowed to be created without a first or last name.

Backtracking in the Code

First, I checked what is being returned by this line:

var volunteer = taskSignup.User;  

taskSignup.User is an ApplicationUser

public class TaskSignup  
{
    public int Id { get; set; }
    public int TaskId { get; set; }
    public AllReadyTask Task { get; set; }
    public ApplicationUser User { get; set; }
    public string AdditionalInfo { get; set; }
    public DateTime StatusDateTimeUtc { get; set; } = DateTime.UtcNow;
    public TaskStatus Status { get; set; }
    public string StatusDescription { get; set; }
    public int? ItineraryId { get; set; }
    public Itinerary Itinerary { get; set; }
}

The ApplicationUser class inherits from Microsoft.AspNetCore.Identity.EntityFrameworkCore's IdentityUser. So basically, ApplicationUser is a way to identify a user in the system.

Looking at the ApplicationUser class, it looks to be an EF controlled entity:

public class ApplicationUser : IdentityUser  
{
    [Display(Name = "Associated skills")]
    public List<UserSkill> AssociatedSkills { get; set; } = new List<UserSkill>();
    public string FirstName { get; set; }
    public string LastName { get; set; }
}

But FirstName and LastName were not decorated with the [Required] attribute, so if I had stopped here, I still would have assumed that creating an Application user with a null FirstName and LastName was OK.

But I wanted to see how an ApplicationUser was created in the system. It still seemed odd to me that we would allow a user to be created without a first and last name.

I found where we were creating the ApplicationUser, and it was in one of our controllers:

[HttpPost]
[AllowAnonymous]
[ValidateAntiForgeryToken]
public async Task<IActionResult> Register(RegisterViewModel model)  
{
    if (ModelState.IsValid)
    {
        var user = new ApplicationUser
        {
            FirstName = model.FirstName,
            LastName = model.LastName,
            UserName = model.Email,
            Email = model.Email,
            PhoneNumber = model.PhoneNumber,
            TimeZoneId = _generalSettings.Value.DefaultTimeZone
        };

        var result = await _userManager.CreateAsync(user, model.Password);

        ...
    }
}

Okay, so we're checking ModelState.IsValid here, let's take a look at the model, RegisterViewModel:

public class RegisterViewModel  
{
    [Required]
    [Display(Name = "First Name")]
    public string FirstName{ get; set; }

    [Required]
    [Display(Name = "Last Name")]
    public string LastName { get; set; }

    ...
}

Aha, found it!

The FirstName and LastName are required when creating a new ApplicationUser, which in turn, means we should never have to check for null or empty strings when querying for that data as we're validating it up front.

Now that I know it's impossible to create an ApplicationUser without a FirstName and LastName, I can update my code to:

var subject = "{volunteer.FirstName} {volunteer.LastName}";  

and even better, get rid of the subject var altogether. This:

var command = new NotifyVolunteersCommand  
{
    ViewModel = new NotifyVolunteersViewModel
    {
        EmailMessage = message,
        HtmlMessage = message,
        EmailRecipients = new List<string> { adminEmail },
        Subject = subject
    }
};

now becomes:

var command = new NotifyVolunteersCommand  
{
    ViewModel = new NotifyVolunteersViewModel
    {
        EmailMessage = message,
        HtmlMessage = message,
        EmailRecipients = new List<string> { adminEmail },
        Subject = $"{volunteer.FirstName} {volunteer.LastName}"
    }
};

There is one more thing I would change here, and that is the [Required] attributes should be put onto the EF managed entity itself (ApplicationUser) instead of on RegisterViewModel. That allows people looking at this entity to see right away that FirstName and LastName cannot be null without backtracking through the code like I did.

In Closing

That was fun, right? (that is a rhetorical question)

Be the developer that tracks this type of stuff down and makes code in the system explicit instead of being the developer who wraps everything with null or string.IsNullOrEmpty checks.

If you choose to be the latter, you're basically putting the cognitive load on every new developer coming into the system to do the work that I just did in this blog, when simply changing the query/subject code to use the FirstName and LastName variables without the null checks would have been enough to inform the developer that we always expect a value

Michael McCarthy

Read more posts by this author.