What I learned after 1000 code review

While working on LinkedIn, most of my work was code review. It turned out that I gave some recommendations many times, so I decided to make a list that I shared with the team.

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

Recommendation 1: Throw exceptions if something goes wrong

Often the pattern looks like this:

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

This pattern caused interruptions in one of the mobile applications I was working on. The server-side search we used started throwing exceptions. It turned out that there was some code on the server API of the application similar to the one above. Therefore, the application received 200 server responses and happily showed an empty list for each search request.

If instead the API threw an exception, then our monitoring systems would immediately process it and fix it.

There are many cases where it may be tempting to simply return an empty object after you catch the exception. Examples of empty objects in Java are Optional.empty (), null, and an empty list. One of the cases where one wants to return a null value is URL parsing. Instead of returning null if the URL cannot be obtained from a string, ask yourself: “Why is the URL incorrect? Is this a data problem that we must correct on the input stream? ”

Empty objects are not a suitable tool for this job. If the situation is exceptional, you should throw an exception

Recommendation 2: Use the most specific data types.

This recommendation is an alternative to stringly typed programming .

Too often, I see code like this:

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 avoids a number of errors and is mainly the main reason for choosing a strongly typed language such as Java.

So now the question is: how do successful programmers end up writing bad code with strong typing? Answer: because the outside world is not very typified. There are several different places where the program gets the lines from, for example:

• request parameters and paths in urls
• Json
• databases that do not support enumerations
• poorly written libraries

In all these cases, you should use the following strategy to avoid this problem: process and serialize the lines at the beginning and end of the program. Here is an example:

// Step 1: Take a query param representing a company name / member id pair and parse it
// example: context=Pair(linkedin,456)
Pair 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 will provide several advantages. Incorrect data is detected immediately; The application throws an error in advance. In addition, you do not need to catch exceptions during analysis throughout the application after a single data check. In addition, strong typing implies a more intuitive signature of methods and you do not have to write a bunch of javadocs for each of the methods.

Recommendation 3: Use Optional instead of Null

One of the best innovations in Java 8 is the class Optional, which is an object that may or may not exist.

The trivial question is: what exception has its own acronym? Answer: NPE or Null Pointer Exception. This is by far the most common Java exception, often called the billion dollar error .

OptionalAllows you to completely remove NPE from your program. However, it must be used correctly. Here are some tips to
use Optional:

• You should not just call .get ()at any time when you have Optionalto use it, instead carefully think about when Optionalabsent and come up with a rational default value.
• If you do not have rational default values, the methods, how .map ()and .flatMap ()allows to postpone his choice for later.
• If the external library returns NULL to indicate an empty case, immediately wrap the value using Optional.ofNullable (). Believe me, you will thank yourself later. nulls tend to pop up inside programs, so it’s best to stop them at the source.
• Use Optionalas the return value of the method. This is great because you do not need to read javadoc to find out if this value can be omitted.

Bonus Recommendation: “Unlift” Methods When Possible

You should try to avoid methods that look like this:

// AVOID:
CompletableFuture method(CompletableFuture param);
// PREFER: 
T method(S param);
// AVOID:
List method(List param);
// PREFER:
T method(S param);
// AVOID: 
T method(A param1, B param2, Optional 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 these methods have in common? They use container objects such as Optional, List, or Task as method parameters. Even worse, when the return type is the same container (i.e., one method parameter accepts Optional and returns Optional).

Why?
1) Promise A method(Promise B param)
It is less flexible, but easier.

2) A method(B param).

If you have Promise B , you can use the first method, or you can use the second method by using the “Lifting” function .map. (i.e. promise.map(method)).

However, if you only have B, you can easily use the second method, but you cannot use the first, which makes the second method a much more flexible option.

I like to call this technique “unlifting” because it is the opposite of the common functional utility method “lift”. If you rewrite methods in this way, they will become more flexible and simpler when called.

---

The translation was supported by EDISON Software , which professionally integrates Axxon Next and SureView Immix video surveillance systems and creates a useful anti-procrastination application .