Null Checks That Don't Prevent Anything Aren't Craftmanship

Did you ever take a specific precaution to avoid failure, not because something was likely to go wrong, but because the cost of checking felt low? Going back to the house before a flight to make sure the oven is off, packing two phone chargers for a trip because it's 'safer' and you're in a hurry, asking the person you're dating "do you like me?", when nothing suggests otherwise.

It happens in software all the time.

• All reused code are put in @Injectable, or @Autowired services" even if you only need pure functions ("when in doubt, use dependency injection, it's just safer")
• Code comments galore ("I'm preventing confusion, what's wrong with that? Someone could get confused")
• Every nested property in an object, use a non null operator ("if we use it every time, we can't get an Cannot find property 'x' of undefined in production")

I could talk about the benefit of helper functions, and how it becomes less overhead long term. I could repackage and give a speech on why code comments reflect bad code. But the simplest way to call out what I call "psychological saftey" code is with excessive null checking and coverage gaps when we pick apart the code and expose all the outcomes.

Good Null Checking

Null checking is good when there is a real possibility, that an object may be null or undefined (for example, a backend service returns a deeply nested object, and we know the layers are not garunteed to be non-null). It prevents a runtime error that would stop the thread, and it allows the developer to handle situations where we can't get or reach the information we want based on external dependencies.

For example, some business logic relies on a nested property of a complex object returned from an API. From our limited knowledge of the back end, userProfile could be null or undefined, socialMedia could be null or undefined, professionalNetworks... (you get where I'm going with this). If we didn't have our ? non null operators, and any of these were null or undefined, we'd get a runtime error like cannot read property 'socialMedia' of undefined. Instead, if any of the properties are null, we have a fallback, set this.linkedInProfile to "No LinkedIn profile provided".

getOrderDeliveryAddress(): void {
  const userProfile = this.dataService.getUserProfile();
  const linkedInUrl =
    userProfile?.socialMedia?.professionalNetworks?.linkedIn?.profileUrl;

  if (linkedInUrl) {
    this.linkedInProfile = linkedInUrl;
  } else {
    this.linkedInProfile = 'No LinkedIn profile provided';
  }
}

Null checking that's 'Extra fluff'

Now let's take an example where on the surface, syntatically, each item can be null, there won't be a compile error, but on closer inspection, the null checks don't really prevent anything. I'm going to show how often, this may not appear obvious, but when we dig deeper and play with the code, we're actually adding non-usefule code. Below, we have a data service, which will either return array of names from a user service, or return something null or undefined. To extract a unique and alphabetical list of names, we map the array to get a list of names, filter them to ensure they're unique, and sort them so that they're in alphabetical order.

  extractUniqueUserNames(): void {
    const users = this.dataService.getUsers();
    const uniqueNames = users?.map((user) => user?.name ? user.name : '')
      ?.filter((value, index, self) => self.indexOf(value) === index)
      ?.sort();

    if (uniqueNames)
    {
      this.uniqueNames = uniqueNames;
    }
  }

On the surface, the users?.map?(...)?.filter(...)?.sort() chaining is completely coverable, 100% by Jest or Jasmine testing. Every operator can be hit by virtue of the left hand part of the operator being not null or undefined.

const users: User[] = [
  { name: 'John Doe', email: 'john@example.com' },
  { name: 'Jane Smith', email: 'jane@example.com' },
  { name: 'John Doe', email: 'john2@example.com' },
  { name: 'Bob Johnson', email: 'bob@example.com' }
];

mockDataService.getUsers.mockResolvedValue(users);

it('should extract unique user names and sort them', () => {
  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([
    'Bob Johnson',
    'Jane Smith',
    'John Doe'
  ]);
});

it('should handle users with missing names', () => {
  mockDataService.getUsers.mockReturnValue([
    { name: 'John Doe', email: 'john@example.com' },
    { name: '', email: 'noname@example.com' },
    { email: 'noname2@example.com' }
  ]);

  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([
    '',
    'John Doe'
  ]);
});

it('should handle when getUsers returns undefined', () => {
  mockDataService.getUsers.mockReturnValue(undefined);

  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([]);
});

it('should handle when getUsers returns null', () => {
  mockDataService.getUsers.mockReturnValue(null);

  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([]);
});

it('should handle null user elements in the array', () => {
  mockDataService.getUsers.mockReturnValue([
    { name: 'John Doe', email: 'john@example.com' },
    null,
    { name: 'Jane Smith', email: 'jane@example.com' },
    undefined,
    { name: 'Bob Johnson', email: 'bob@example.com' }
  ]);

  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([
    '',
    'Bob Johnson',
    'Jane Smith',
    'John Doe'
  ]);
});

it('should handle users with undefined names', () => {
  mockDataService.getUsers.mockReturnValue([
    { name: 'John Doe', email: 'john@example.com' },
    { name: undefined, email: 'noname@example.com' },
    { name: null, email: 'noname2@example.com' }
  ]);

  component.extractUniqueUserNames();

  expect(component.uniqueNames).toEqual([
    '',
    'John Doe'
  ]);
});

No null runtime exceptions can be thrown, no code uncovered. Pretty straightfoward right? However, if we refactor this code and spread out these non null operators into their own if/else statements, we'll discover that we're adding null operators to safeguard against outcomes that do not exist.

extractUniqueUserNamesWithoutOptionalChaining(): void {
  const users = this.dataService.getUsers();

  if (Array.isArray(users))
  {
    const mappedNames = users.map((user) => user?.name ? user.name : '');

    if (mappedNames !== null && mappedNames !== undefined)
    {
      const uniqueNames = mappedNames.filter((value, index, self) => self.indexOf(value) === index);

      if (uniqueNames !== null && uniqueNames !== undefined)
      {
        const sortedNames = uniqueNames.sort();

        if (sortedNames !== null && sortedNames !== undefined)
        {
          this.uniqueNames = sortedNames;
        } else
        {
          this.errorMessage = 'Sorted names is null or undefined';
        }
      } else
      {
        this.errorMessage = 'Unique names is null or undefined';
      }
    } else
    {
      this.errorMessage = 'Mapped names is null or undefined';
    }
  } else
  {
    this.errorMessage = 'Users is null or undefined or not an array';
  }
}

Let's write tests for every possible outcome:

describe('extractUniqueUserNamesWithoutOptionalChaining', () => {

  it('should extract unique user names and sort them', () => {
    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([
      'Bob Johnson',
      'Jane Smith',
      'John Doe'
    ]);
    expect(component.errorMessage).toBe('');
  });

  it('should handle when getUsers returns undefined', () => {
    mockDataService.getUsers.mockReturnValue(undefined);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([]);
    expect(component.errorMessage).toBe(
      'Users is null or undefined or not an array'
    );
  });

  it('should handle when getUsers returns null', () => {
    mockDataService.getUsers.mockReturnValue(null);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([]);
    expect(component.errorMessage).toBe(
      'Users is null or undefined or not an array'
    );
  });

  it('should handle when getUsers returns a non-array value', () => {
    mockDataService.getUsers.mockReturnValue('not an array' as any);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([]);
    expect(component.errorMessage).toBe(
      'Users is null or undefined or not an array'
    );
  });

  it('should handle users with missing names', () => {
    mockDataService.getUsers.mockReturnValue([
      { name: 'John Doe', email: 'john@example.com' },
      { name: '', email: 'noname@example.com' },
      { email: 'noname2@example.com' }
    ]);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([
      '',
      'John Doe'
    ]);
    expect(component.errorMessage).toBe('');
  });

  it('should handle null user elements in the array', () => {
    mockDataService.getUsers.mockReturnValue([
      { name: 'John Doe', email: 'john@example.com' },
      null,
      { name: 'Jane Smith', email: 'jane@example.com' },
      undefined,
      { name: 'Bob Johnson', email: 'bob@example.com' }
    ]);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([
      '',
      'Bob Johnson',
      'Jane Smith',
      'John Doe'
    ]);
    expect(component.errorMessage).toBe('');
  });

  it('should handle users with undefined names', () => {
    mockDataService.getUsers.mockReturnValue([
      { name: 'John Doe', email: 'john@example.com' },
      { name: undefined, email: 'noname@example.com' },
      { name: null, email: 'noname2@example.com' }
    ]);

    component.extractUniqueUserNamesWithoutOptionalChaining();

    expect(component.uniqueNames).toEqual([
      '',
      'John Doe'
    ]);
    expect(component.errorMessage).toBe('');
  });

});

How is our "expanded" code covered now?

No test can be written to cover all these scenerios. By the time we reach line 65, mappedNames must be an array of either empty or non empty strings. If it's an array by line 69, it's an array by 73, and it's an array by line 59.

But what about empty arrays?

Completely legal to call map, filter, and sort on an empty array. Each operator will still execute in the chain without throwing an error.

It's covered and we have no runtime errors in production, what's the harm?

The argument, "it's not going to hurt anything, and we know for sure there won't be a production issue." Seems utalitarian, and cost efficient. Here's the problem, when more and more of the code decisions stop being delibarate decisions, and instead become a quick win to reduce uncertainty, a precendent is being set: cognitive load regardless of if the outcome (or potential outcome) is meaningful and the behavior in production is ideal. Did I say in an earlier article that patterns to reduce cognitive load are helpful, and that picking "the perfect tool for the job" is excessive cognitive load? Yes. What's different about this is, when a tool is used that will not impact an outcome, using that tool over and over again is outsourcing critical thinking, and the mentality spills over across the code. Nobody can explain why every single peice of shared code is a service when it doesn't have to be, why let is always the default over const when we know that variable will never be changed, and why majority of variables in fuctions are component scoped ie this.foo instead of foo when a pure function will always get the job done.

Better alternative

In situations like these, when tempted to use null checking for every layer of an object, ask youself, how much do I actuall need, and go with that

extractUniqueUserNamesRefactored(): void {
  const users = this.dataService.getUsers();

  const uniqueNames = Array.isArray(users)
    ? users
        .map((user) => user?.name ? user.name : '')
        .filter((value, index, self) => self.indexOf(value) === index)
        .sort()
    : [];

  if (uniqueNames.length > 0)
  {
    this.uniqueNames = uniqueNames;
  }
}

At this point, we've reduced uncertainty to a signle boundary: whether users is an array. Once that condition is satisfied, the rest of the operation is deterministic and safe.