CodeSOD: Checking Your Options

If nulls are a billion dollar mistake", then optional/nullable values are the $50 of material from the hardware store that you use to cover up that mistake. It hasn't really fixed anything, but if you're handy, you can avoid worrying too much about null references.

D. Dam Wichers found some interesting" Java code that leverages optionals, and combines them with the other newish Java feature that everyone loves to misuse: streams.

First, let's take a look at the right" way to do this though. The code needs to take a list of active sessions, filter out any older than a certain threshold, and then summarize them together into a single composite session object. This is a pretty standard filter/reduce scenario, and in Java, you might write it something like this:

return sessions.stream() .filter(this::filterOldSessions) .reduce(this::reduceByStatus);

The this::... syntax is Java's way of passing references to methods around, which isn't a replacement for lambdas but is often easier to use in Java. The stream call starts a stream builder, and then we attach the filter and reduce operations. One of the key advantages here is that this can be lazily evaluated, so we haven't actually filtered yet. This also might not actually return anything, so the result is implicitly wrapped in an Optional type.

With the right" way firmly in mind, let's look at the body of a method D. Dam found.

 Optional<CachedSession> theSession; theSession = sessions.stream() .filter(session -> filterOldSessions(session)) .reduce((first, second) -> reduceByStatus(first, second)); if (theSession.isPresent()) { return Optional.of(theSession.get()); } else { return Optional.empty(); }

This code isn't wrong, it just highlights a developer unfamiliar with their tools. First, note the use of lambdas instead of the this::... syntax. It's functionally the same, but this is harder to read- it's less clear.

The real confusion, though, is after they've gotten the result. They understand that the stream operation has returned an Optional. So they check if that Optional isPresent- if it has a value. If it does, they get the value and wrap it in a new Optional (Optional.of is a static factory method which generates new Optionals). Otherwise, if it's empty, we return an empty optional. Which, if they'd just returned the result of the stream operation, they would have gotten the same result.

It's always frustrating to see this kind of code. It's a developer who is so close to getting it, but who just isn't quite there yet. That said, it's not all bad, as D. Dam points out:

In defense of the original code: it is a little more clear that an Optional is setup properly and returned.

I'm not sure that it's necessary to make that clear, but this code isn't bad, it's just annoying. It's the kind of thing that you need to bring up in a code review, but somebody's going to think you're nit-picking, and when you start using words like readability, there'll always be a manager who just wants this commit in production yesterday and says, Readability is different for everyone, it's fine."

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