What I Learned From Doing 1000 Code Reviews

72,313 reads

@ stevenheidel Steven Heidel I develop software and develop people. https://stevenheidel.com

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.

reactions

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

reactions

Suggestion 1: Throw an exception when things go wrong

A common pattern I have seen is this:

reactions

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.

reactions

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

reactions

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:

reactions

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

reactions

Suggestion 2: Use the most specific type possible

This suggestion is basically the opposite of stringly typed programming.

reactions

Too often I see code like these examples:

reactions

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.

reactions

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:

reactions

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.

reactions

Here’s an example:

reactions

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

reactions

Suggestion 3: Use Optionals instead of nulls

One of the best features to come out of Java 8 is the

Optional

reactions

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.

reactions

Optional

Optional

reactions

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.

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 .map() and .flatMap() allow you to defer this decision until later.

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

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

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

You should try to avoid methods that look like this:

reactions

// AVOID: CompletableFuture<T> method (CompletableFuture<S> param) ; // PREFER: T method (S param) ; // AVOID: List<T> method (List<S> param) ; // PREFER: T method (S param) ; // AVOID: T method (A param1, B param2, Optional<C> param3) ; // PREFER: 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).

reactions

Why?

1)

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

reactions

is less flexible than simply having

2)

A method(B param)

reactions

If you have a

Promise<B>

.map

promise.map(method)

reactions

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.

reactions

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.

reactions

See Also

Share this story @ stevenheidel Steven Heidel Read my stories I develop software and develop people. https://stevenheidel.com

Tags