CodeSOD: WWJSD?

A few months ago, Lee was reviewing a pull request from Eddie. Eddie was a self-appointed "rockstar" developer. Eddie might be a "junior" developer in job title, but they're up on the latest, greatest, bestest practices, and if everyone would just get out of Eddie's way, the software would get so much better.

Which is why the pull request Lee was looking at touched nearly every file. Every change was some variation of this:

- if (obj == null)+ if (obj is null)

"Did... did you do a 'Replace in Files' on the entire solution?" Lee asked.

"Well, I had to use the regex find-and-replace," Eddie said, "have you ever used that? It's a great refactoring tool, you can get it with control-H, and then you have to check a box-"

"I know how it works," Lee said. "Why?"

"Well, a regex, or a regular expression is a-"

"No, why did you make the change?"

"Oh! Well, that's the correct way to check for nulls. The equality operator could be overloaded, and that overload might have bugs."

"I mean, it's a correct way," Lee said, "but the equality operator is also fine. Any bugs in our overloads are likely already caught. There's no reason to make a change like this through the whole codebase."

The debate went on for longer than it should have. Eventually, Eddie defended his choice by saying, "Well, it's the way Jon Skeet does it," at which point Lee threw up his hands.

"Fine." Lee approved the request. After all, what was the worst that could happen?

What happened was that a month or two later, the application started throwing NullReferenceExceptions in production. The change which caused the behavior was on line:

Coordinate p = maybeGetCoordinate();if (p != null){ p.Project(alternateCoordinateSystem); // NullReferenceException}

Look at that, someone using the more traditional equality operator! Maybe Eddie had a point, maybe an overloaded equality operator was the problem. Certainly, the bug went away if Lee used !(p is null) (or, in C# 9.0, p is not null).

Still, it would be good to fix the bug. Lee took a peek at the overloaded operators:

public static bool operator == (Coordinate pt1, Coordinate pt2) => !(pt1 is null) && !(pt2 is null) && pt1.Longitude == pt2.Longitude && pt1.Latitude == pt2.Latitude;public static bool operator !=(Coordinate pt1, Coordinate pt2) => !(pt1 == pt2);

The key bug is !(pt1 is null) && !(pt2 is null)- if one or more of the operands is null, then return false. This means null == null is always false, which means the p != null check returns false if p is null.

As it turned out, Eddie did have a point about favoring is null checks. Someone might add a buggy operator overload which could cause unexpected behavior. Who did add that particular buggy overload? Why, Eddie, of course.

Lee submitted a patch, and gently suggested Eddie might need to spend a little more time in the woodshed before declaring themselves a rockstar.

[Advertisement] BuildMaster allows you to create a self-service release management platform that allows different teams to manage their applications. Explore how!