CodeSOD: Retern your Linter

Today, let's just start with a single line of Java code, from Rich.

T oldValue = (T) (configItems.get(configKey) == null ? null : configItems.get(configKey));

This is a pretty standard bad ternary. If a value is null, we set a variable equal to null, if it's non-null, we set a variable to its value. So this whole thing really does nothing, and could just be a call to configItems.get.

Except it's actually worse. This application is multi-threaded, and configItems is a standard Java HashMap, which is not thread safe. This creates all sorts of fun race conditions, and in this specific case, it means that oldValue could be set to null when the value at configKey is actually set, which triggered a bunch of downstream bugs in actual practice.

Except that it's actually worse. Because this line of code wasn't always like this. It used to look like this:

T oldValue = (T) (configItems.get(configKey) != null ? configItems.get(configKey) : null);

This is the initial version of the line, which as you can see, just reverses the condition. It's otherwise the same line. This line was sitting in production code for a big chunk of the product's lifetime, until one day when the team added Checkstyle and PMD rules to their project. Those linting rules didn't like the original version of this line, so one of Rich's peers "fixed" the line by futzing with it until the linter passed it.

So the initial line was a mistake. Linting rules, quite helpfully, drew attention to that mistake. A programmer looked at that line, and their best approach to fixing that mistake was just to reverse it.

Rich sums up: "I guess this is proof that linting rules only get you so far. Remember, only YOU can prevent terrible code!"

[Advertisement] Utilize BuildMaster to release your software with confidence, at the pace your business demands. Download today!