07.12.2017 08:55
rishat_edison 18 13700 Источник
Во время работы в LinkedIn большая часть моей работы составляло код-ревью. Вышло так, что некоторые рекомендации я давал много раз, поэтому я решил составить список, которым поделился с командой.

Вот мои 3 (+1 бонусная) наиболее распространенные рекомендации по код-ревью.

image

Рекомендация 1: Выбрасывайте исключения, если что то идет не так


Зачастую паттерн выглядит так:

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


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

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

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

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

Рекомендация 2: Пользуйтесь наиболее специфическими типами данных


Эта рекомендация — альтернатива stringly typed programming.

Слишком часто я вижу такой код:

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
  • базы данных, которые не поддерживают перечисления
  • плохо написанные библиотеки


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

// 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 для каждого из методов.

Рекомендация 3: Используйте Optional вместо Null


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

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

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

  • Вы не должны просто вызывать .get () в любое время, когда у вас есть Optional, чтобы использовать его, вместо этого тщательно подумайте о том случае, когда Optional отсутствует и придумайте рациональное значение по умолчанию.
  • Если у вас еще нет рационального значения по умолчанию, то методы, как .map () и .flatMap () позволит отложить его выбор на потом.
  • Если внешняя библиотека возвращает NULL, чтобы указать пустой кейс, сразу же оберните значение используя Optional.ofNullable (). Поверьте мне, вы поблагодарите себя позже. nulls имеют тенденцию «всплывать» внутри программ, поэтому лучше всего остановить их в источнике.
  • Используйте 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 , вы можете использовать первый способ, или вы можете использовать второй способ путем «Lifting» функции с помощью .map. (т.е. promise.map(method)).

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

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


Перевод выполнен при поддержке компании EDISON Software, которая профессионально занимается интеграцией систем видеонаблюдения Axxon Next и SureView Immix и создает полезное приложение против прокрастинации.

Комментарии (18)


  1. Throwable
    07.12.2017 12:47
    #10556004
    +2

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

    Между прочим это один из кейсов негативного влияния checked exceptions. Если бы RemoteInvocationException не был бы задекларирован как checked, программист бы скорей всего не допустил ошибки. Вообще по эксепшнам можно отдельно статью писать. Смысл простой по возможности избегайте checked-exceptions. Всегда врапьте checked-exceptions по возможности в один из стандатрных форм RuntimeException: IllegalArgumentException, IllegalStateException, etc...


    Пользуйтесь наиболее специфическими типами данных

    Более того, в некоторых случаях имеет смысл использовать класс-враппер над типом, вместо самого типа. Например, вместо String phoneNumber лучше сделать враппер-класс PhoneNumber с единственным полем String value. Для перечислимых типов по возможности используйте enums.


    Используйте Optional вместо Null

    Сомнительная рекомендация. Optional к Java прикрутили сбоку совсем недавно с приходом элементов функционального программирования, и его использование не должно выходить за рамки функционального "однострочника". Кроме того большинство библиотек и спецификация Java Beans не предусматривает использование Optional вообще. Для null-checking уже давно имеются @Nullable аннотации, которые являются лучшей альтернативой, нежели Optional. Благо тулинг их понимает из коробки. Если же хотите писать в труЪ функциональном стиле с монадами Try и Option, используйте замечательную библиотеку http://www.vavr.io/


    Добавлю от себя еще некоторые правила:


    • По возможности используйте immutable-объекты. Их состояние детерминировано в любой момент.
    • Насчет пустых объектов: они всегда являются лучшей альтернативой, нежели nullable. Особенно важно для коллекций: возвращайте Collections.emptyList() вместо null. Для String использование по дефолту пустой строки "" вместо null избавить от кучи ненужных проверок.
    • Для collection-полей неплохой практикой будет возвращать в геттере Stream вместо Collection.


    1. poxvuibr
      07.12.2017 12:52
      #10556014
      +1

      Особенно важно для коллекций: возвращайте Collections.emptyList() вместо null.

      Мотивация у этого предложения та же, что у рекомендации использовать Optional?


      Для collection-полей неплохой практикой будет возвращать в геттере Stream вместо Collection.

      I'm a lazy girl, in a lazy world? :)


      1. Hokum
        07.12.2017 13:36
        #10556118

        Особенно важно для коллекций: возвращайте Collections.emptyList() вместо null.
        Мотивация у этого предложения та же, что у рекомендации использовать Optional?

        На мой взгляд возвращать Optional имеет смысл когда в результате действия мы можем ничего не вернуть. Если говорить о коллекциях — нам есть разница пустая коллекция или "ничего" (тоже самое касательно строки, пустая строка — может быть валидным результатом). Так как пустая коллекция сама по себе тоже может быть результатом. При этом нельзя рассматривать пустой Optional как ошибку выполнения.


        1. poxvuibr
          07.12.2017 13:45
          #10556140
          +1

          На мой взгляд возвращать Optional имеет смысл когда в результате действия мы можем ничего не вернуть.

          Как я понимаю, Optional имеет смысл возвращать, чтобы не писать код типа.


          if (str != null) {
    result += str.replace('s','f');
}

          а писать так


           result += optional.map(s -> s.replace('s','f')).orElseGet("");

          NPE исключен


          А в случае с листами — если мы уверены, что коллекция никогда не null — можно не делать проверку перед тем, как итерироваться по ней. Идея в общем та же.


          1. Throwable
            07.12.2017 16:17
            #10556508

            Не совсем понимаю чем вторая запись лучше и понятней первой. Если str объявлен как @Nullable, то при возможном NPE тулинг выдаст варнинг — и этого в большинстве случаев достаточно. Кроме того, тулинг достаточно умный, чтобы выводить @Nullable по коду.


            Кроме того, в данном случае, как я и говорил, имеет смысл вообще использовать @Nonnull String str = "" вместо null или Optional. И никаких проверок делать не надо.


            NPE исключен

            Да ладно?


            Optional<String> calculate() {
    return null;
}
Optional<String> str = calculate();
...


            1. poxvuibr
              07.12.2017 16:50
              #10556590

              NPE исключен

              Да ладно?



              И ведь не поспоришь… С другой стороны по уму Optional не может быть null никогда. Это можно использовать при создании правил статического анализа кода.


        1. Throwable
          07.12.2017 15:58
          #10556470

          Если у нас есть сложная операция или композиция, валидным результатом которой может быть "отсутствие результата", то Optional может подходить. Пример: Optional findPerson(query). Использовать же Optional для хранения или передачи состояния — плохая практика. То есть поля объекта Optional или Optional-параметр в сеттере — это сразу плохо (даже Idea вам об этом скажет). Поэтому единственный юзкейс — возврат значения, которого может не быть. Однако Optional в Java — достаточно лимитированная монада. По-сути единственное, что можно с ней сделать — это orElseXXX() (а много логики в map не запихнешь), никакой дальнейшей композиции с ней сделать нельзя (в Java 9 ей добавили свойства стрима и теперь можно делать flatMap()). Кроме того, в большинстве случаев логика требует нечто большее, чем orElse, поэтому все-равно приходится ставить if (maybe.isPresent()). Так что на мой взгляд, не фиг функционально выпендриваться на нефункциональном языке — в Java есть аннотация @Nullable.
          Насчет коллекций — формально да, но реально сложно найти пример, где бы необходимо было именно Nullable-коллекция. Пустая коллекция — это и есть отсутствие результата. Какой смысл при этом будет иметь возврат null (или Optional) не совсем ясно.


          1. Hokum
            07.12.2017 16:43
            #10556582
            +1

            Действительно, с коллекцией я перемудрил. Что-то в голову не приходит когда вместо пустой коллекции была бы необходимость вернуть null.


            1. Sayonji
              07.12.2017 20:35
              #10556940

              Кеш какой-нибудь.


          1. NikitOS9
            08.12.2017 12:26
            #10557678

            >Поэтому единственный юзкейс — возврат значения, которого может не быть
            всмысле optional логическая абстракция чтобы сказать что тут значения может не быть и это нормально…
            так null то же яйцо вид сбоку


    1. javax
      07.12.2017 14:16
      #10556194

      и его использование не должно выходить за рамки функционального «однострочника


      Не согласен. Если Вы не пишете на Котлине, на котором явно можно сказать, что значение не может быть Null, то очень удобно с помощью Optional явно давать понять — может это значение есть, а может быть его и нет


    1. izzholtik
      07.12.2017 16:30
      #10556554

      Когда я вижу, что код кидает непроверяемые исключения, мне хочется плакать, потому что я ещё ни разу не видел, чтобы с ними обращались аккуратно. Обязательно любая библиотека, использующая их, бросает пару исключений, не описанных в JavaDoc'ах, и всё заканчивается в оборачивании любых вызовов такого кода в try-catch с перехватом RuntimeException, чтобы ни дай бог опять оно не выстрелило. Я понимаю, почему люди не любят проверяемые исключения, но то, как реализованы (и как используются) непроверяемые, создаёт дикое количество ошибок, зачастую заканчивающихся падением всей программы.


      1. dzugaru
        07.12.2017 21:46
        #10557000

        Если у вас где-то в коде перехват рантайм экцепшнов, то а) у меня для вас плохие новости б) это меньшая из ваших проблем


        1. izzholtik
          08.12.2017 00:02
          #10557074

          Ну а что я могу сделать, если Javadoc — это те же комментарии, а комментариям свойственно протухать?
          Хорошо, какой способ вы предлагаете?


  1. poxvuibr
    07.12.2017 12:48
    #10556006
    +1

    Последний совет по поводу unlift без объяснения вообще непонятен.
    Вот эта конструкция


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

    По идее возвращает фьючу, которая инкапсулирует результат процесса, который произойдёт после того, как закончится фьюча во входном параметре.
    То есть, с учётом рекомендации автора нужно писать не так


    CompletableFuture<Stiring> future = method(param);

    А вот так


    CompletableFuture<Stiring> future = param.thenApply(method);

    С листами не


    List<String> list =  method(param);

    а


    List<String> list = list.stream.map(method).collect(Collectors.toList());

    С Optional история видимо другая. Автор просто не хочет передавать Optional в качестве параметра. Тут видимо вместо.


    String res = method(optional);

    предлагается


    String res = method(optional.getOrElse("default value"));


  1. rraderio
    07.12.2017 15:15
    #10556320

    // This method is clearly doing two things, it should be two methods

    Почему тогда 2 метода с одинаковым именем?


  1. vbabich
    07.12.2017 18:01
    #10556732
    +1

    язык не «со строковой типизацией» а «со строгой типизацией»


  1. zoonman
    08.12.2017 00:11
    #10557078

    Рекомендация 1: Выбрасывайте исключения, если что то идет не так

    Отслеживайте исключения, когда что-то идет не так.
    Не следует рушить приложение и терять все данные или выбрасывать кишки наружу.
    Если что-то пошло не так, запишите это в лог, а пользователю выдайте либо вежливое сообщение об ошибке, либо просто верните результат по умолчанию (пустой список из примера).