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 recommendations for code review.


')

Recommendation 1: Throw out exceptions if something goes wrong

Often the pattern looks like this:

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

This pattern caused interruptions in one of the mobile apps I worked on. A search on the server side that we used started throwing out exceptions. It turned out that the server API of the application had some code similar to the one above. Therefore, the application received 200 server responses and happily showed an empty list for each search query.

If, instead, the API threw an exception, 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 an exception. Examples of empty objects in Java are Optional.empty (), null, and an empty list. One of the cases where you want to return a null value is parsing the URL. Instead of returning null if the URL cannot be obtained from the string, ask yourself: “Why is the URL wrong? Is this a problem with the data we need to fix on the input stream? ”

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

Recommendation 2: Use the most specific data types.

This recommendation is an alternative to stringly typed programming .

Too often, I see this code:

 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 basically the main reason for choosing a language with strong typing, such as Java.

So now the question arises: how do successful programmers end up writing bad code with strict typing? Answer: because the outside world is not strongly typed. There are several different places from which the program receives lines, for example:

• query parameters and URL paths
• Json
• databases that do not support enumerations
• badly written libraries

In all these cases, you should use the following strategy to avoid this problem: process and serialize 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<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 will give a number of advantages. Incorrect data is detected immediately; The application gives an error in advance. In addition, you do not need to intercept exceptions while analyzing the entire application after a single data check. In addition, strong typing implies a more obvious method signature and you don't 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 Optional class, which is an object that may or may not exist.

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

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

• You should not just call .get () at any time when you have an Optional to use it, instead think carefully about when the Optional absent and come up with a rational default value.
• If you do not have a rational default value yet, then methods like .map () and .flatMap () will allow you to postpone his choice for later.
• If the external library returns NULL to specify an empty case, immediately wrap the value using Optional.ofNullable () . Believe me, thank you later. Nulls tend to “float” inside programs, so it’s best to stop them at the source.
• Use Optional as the return value of the method. This is great because you do not need to read javadoc to find out if you can omit this value.

Bonus recommendation: “Unlift” methods when possible

You should try to avoid methods that look like this:

 // 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 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 an Optional and returns an Optional).

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

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 using the .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 one, 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 lift method. If you rewrite methods in this way, they will become more flexible and easier to call.

---

The translation was made with the support of EDISON Software , which is professionally engaged in the integration of Axxon Next and SureView Immix video surveillance systems and creates a useful anti-procrastination application .