Steven Heidel

@stevenheidel

What I learned from doing 1000 code reviews

While working at LinkedIn a large part of my job involved doing code reviews. There were certain suggestions that kept coming up over and over again, so I decided to put together a list that I shared with the team.

Here are my 3 (+1 bonus) most common code review suggestions.

Suggestion 1: Throw an exception when things go wrong

A common pattern I have seen is this:

This pattern actually caused an outage in one of the mobile apps I worked on. The search backend we were using started throwing exceptions. However, the app’s API server had some code similar to the above. Therefore, from the perspective of the app it was getting a 200 successful response and happily showed an empty list for every single search query.

If instead the API had thrown an exception, then our monitoring systems would have picked this up immediately and it would have been fixed.

There are lots of times when it may be tempting to just return an empty object after you’ve caught an exception. Examples of empty objects in Java are Optional.empty(), null, and empty list. A place where this comes up all the time is in URL parsing. Instead of returning null if the URL can’t be parsed from a String, ask yourself: “Why is the URL malformed? Is this a data issue we should be fixing upstream?”.

Empty objects are not the right tool for this job. If something is exceptional you should throw an exception.

Suggestion 2: Use the most specific type possible

This suggestion is basically the opposite of stringly typed programming.

Too often I see code like these examples:

Using the most specific type possible allows you to avoid a whole class of bugs and is basically the entire reason for choosing strongly typed language like Java in the first place.

So now the question is: how do well-intentioned programmers end up writing bad stringly typed code? The answer: because the external world is not strongly typed. There are a number of different places where strings come from, such as:

  • query and path parameters in urls
  • JSON
  • databases that don’t support enums
  • poorly written libraries

In all these cases, you should use the following strategy to avoid this problem: keep string parsing and serialization to the fringes of your program. Here’s an example:

This confers a number of advantages. Malformed data is found immediately; the application fails early if there are any problems. You also don’t have to keep catching parsing exceptions throughout the entire application once the data has been validated once. In addition, strong types make for more descriptive method signatures; you don’t need to write as many javadocs on every method.

Suggestion 3: Use Optionals instead of nulls

One of the best features to come out of Java 8 is the Optional class that represents an entity which could reasonably exist or not exist.

Trivia question: what is the only exception to have its own acronym? Answer: a NPE or Null Pointer Exception. It is by far the most common exception in Java and has been referred to as a billion dollar mistake.

Optional allows you to completely remove NPEs from your program. However, it must be used correctly. Here’s some advice surrounding the
use of Optional:

  • You should not simply call .get() anytime you have an Optional in order to use it, instead think carefully about the case where the Optional is not present and come up with a sensible default value.
  • If you do not yet have a sensible default value then methods like .map() and .flatMap() allow you to defer this decision until later.
  • If an external library returns null to indicate the empty case, then immediately wrap the value using Optional.ofNullable(). Trust me, you will thank yourself later. nulls have a tendency to “bubble up” inside programs so it’s best to stop them at the source.
  • Use Optional in the return type of methods. This is great because then you don’t need to read the javadoc to figure out whether it’s possible for the value to not be present.

Bonus Suggestion: “Unlift” methods whenever possible

You should try to avoid methods that look like this:

What do all the avoid methods have in common? They are using container objects like Optional, List, or Task as method parameters. It’s even worse when the return type is the same kind of container (ie. a one param methods takes an Optional and returns an Optional).

Why?
1) Promise<A> method(Promise<B> param)
is less flexible than simply having

2) A method(B param).

If you have a Promise<B> then you can use 1) or you can use 2) by “lifting” the function with .map. (ie. promise.map(method)).

However, if you have just B then you can easily use 2) but you can’t use 1), which makes 2) the much more flexible option.

I like to call this “unlifting” because it is the opposite of the common functional utility method “lift”. Applying these rewrites make methods more flexible and easier to use for callers.

See Also

Check out my other article on “Practical Functional Programming”: https://hackernoon.com/practical-functional-programming-6d7932abc58b

This article has been translated into Korean by Ilkwon Sim.

This article has also been translated into Chinese by DORA.

More by Steven Heidel

Topics of interest

More Related Stories