What I Learned From Doing 1000 Code Reviews

来自:Hacker Noon 2020-01-09

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:

List<String> getSearchResults(...) {
  try {
    List<String> results = // make REST call to search service
    return results;
  } catch (RemoteInvocationException e) {
    return Collections.emptyList();

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:

void doOperation(String opType, Data data); 
// where opType is "insert", "append", or "delete", this should have clearly been an enum

String fetchWebsite(String url);
// where url is "https://google.com", this should have been an URN

String parseId(Input input);
// the return type is String but ids are actually Longs like "6345789"

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:

// Step 1: Take a query param representing a company name / member id pair and parse it
// example: context=Pair(linkedin,456)
Pair<String, Long> companyMember = parseQueryParam("context");
// this should throw an exception if malformed

// Step 2: Do all the stuff in your application
// MOST if not all of your code should live in this area

// Step 3: Convert the parameter back into a String at the very end if necessary
String redirectLink = serializeQueryParam("context");

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
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 .


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

use of



You should not simply call 
anytime you have an
in order to use it, instead think carefully about the case where the
is not present and come up with a sensible default value. If you do not yet have a sensible default value then methods like 
allow you to defer this decision until later. If an external library returns
to indicate the empty case, then immediately wrap the value using
. 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
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:

CompletableFuture<T> method(CompletableFuture<S> param);
T method(S param);

List<T> method(List<S> param);
T method(S param);

// AVOID: 
T method(A param1, B param2, Optional<C> param3);
T method(A param1, B param2, C param3);
T method(A param1, B param2);
// This method is clearly doing two things, it should be two methods
// The same is true for boolean parameters

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).



Promise<A> method(Promise<B> param)

is less flexible than simply having

A method(B param)
. If you have a
then you can use 1) or you can use 2) by “lifting” the function with 
. (ie.

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.