Правила, которые я выработал по результатам тысяч code review

Во время работы в LinkedIn большая часть моей работы состояла из проверки кода, code review. Были определенные ситуации, которые постоянно возникали снова и снова, поэтому я решил составить список, которым и поделился с командой.

Вот мои 3 (+1 бонус) наиболее распространенных правки, которые я делал во время code review.

Правило code review 1: Генерирование исключения, когда что-то идет не так

Обычно я видел такое:

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

Этот код вызвал сбои в одном из мобильных приложений, над которым я работал. Поисковый бэкенд, который мы использовали, стал выбрасывать исключения. Тем не менее, в приложении имелся код, подобный этому. Поэтому, с точки зрения приложения, оно получало успешный ответ 200 и с радостью показывало пустой список для каждого поискового запроса.

Если бы вместо этого API выбросил исключение, то наша система мониторинга немедленно подобрала бы его, обработала и мы ошибку исправили.

Во многих случаях возникает соблазн просто вернуть пустой объект после того, как вы поймали исключение. Примерами пустых объектов в Java являются Optional.empty(), нулевой или пустой список. Хорошим примером того, где это все встречается постоянно, является парсинг URL. Вместо того, чтобы возвращать null, если URL-адрес не может быть получен из строки, спросите себя: «Почему URL-адрес неправильно сформирован? Является ли это проблемой данных, которую мы должны исправить где-то выше?».

Пустые объекты не являются подходящим инструментом для работы. Если случилось что-то исключительное, то вы должны выбросить исключение.

Правило code review 2: Использование наиболее конкретного типа

Это предложение в основном противоречит строгому типизированному программированию.

Слишком часто я видел код, подобный этому примеру:

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"

Использование наиболее конкретного типа позволяет избежать целого класса ошибок и, в основном, является основной причиной выбора строго типизированного языка, такого как Java.

Внимание, вопрос: как опытные программисты в конечном итоге пишут плохой типизированный код? Ответ: потому что внешний мир не сильно типизирован. Есть несколько разных мест, откуда берутся строки, например:

• параметры запроса и пути в URL-адресах
• JSON
• Базы данных, которые не поддерживают enum
• Плохо написанные библиотеки

В любом таком случае вы должны использовать следующую стратегию, чтобы избежать проблем — сохранить парсинг строки и сериализацию в вашей программе. Вот пример:

// 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");

Это дает ряд преимуществ. Неправильные данные немедленно обнаруживаются; в случае возникновении проблем приложение падает раньше. Кроме того, вам не нужно сохранять исключения для парсинга по всему приложению, так данные проверяются один раз. Кроме того, сильные типы сами по себе более полно описывают методы и вам не надо писать javadocs для каждого метода.

Правило code review 3: Использование Optionals вместо null

Одна из лучших функций Java 8 — это класс Optional, который представляет собой объект, который может существовать или не существовать.

Вопрос на миллион долларов: какое единственное исключение имеет собственную аббревиатуру? Ответ: NPE или Null Pointer Exception. Это, безусловно, самое распространенное исключение в  Java и, конечно, ошибка, которая стоит миллион долларов.

Optional позволяет вам полностью устранить NPE. Однако его следует использовать правильно. Вот некоторые советы по работе с Optional:

• Не надо просто называть .get () в любое время, когда вам надо использовать Optional. Вместо этого внимательно подумайте о том случае, когда Optional не представлен, и придумайте разумное значение по умолчанию.
• Если у вас еще нет разумного значения по умолчанию, тогда такие методы, как .map () и .flatMap (), позволяют отложить это решение.
• Если внешняя библиотека возвращает значение NULL, чтобы указать на пустой случай, сразу же оберните значение с помощью параметра Optional.ofNullable (). Поверьте мне, вы поблагодарите себя позже. Нули имеют тенденцию «всплывать» внутри программ, поэтому лучше остановить их в первоисточнике.
• Используйте Optional как возвращаемый тип в методах. Это здорово, потому что вам не нужно будет читать javadoc, чтобы выяснить, возможно ли, чтобы значение было пустым.

Бонус: Использование Unlift методов, когда это возможно

Вы должны избегать методов, которые выглядят следующим образом:

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

Что общего у всех этих методов? Они используют контейнерные объекты, такие как Optional, List или Task как параметры методов. Еще хуже, когда  возвращаемый тип является тем же самым (т.е. метод принимает Optional и возвращает Optional).

Почему это плохо?

1. Promise<A> method(Promise<B> param)
   менее гибко, чем просто
2. A method(B param)

Если у вас есть Promise <B>, вы можете использовать 1, или вы можете использовать 2 путем «подъема» функции с помощью .map. (т.е. promise.map(method)).

Однако, если у вас есть только B, вы можете легко использовать 2, но вы не можете использовать 1, что делает 2 гораздо более гибким вариантом.

Мне нравится называть это «неподъемным», потому что это противоположность общепринятому функциональному методу «подъем». Применение этих знаний делает методы более гибкими и удобными для использования.