28.01.2022 12:29
e_Hector 36 5300 Источник

Лучший код — это ненаписанный код

Салют, коллеги.

Лично я, очень люблю поговорить про качество, поддерживаемость и выразительность кода (эти умные слова, часто звучат на код ревью)

К сожалению, такие разговоры часто и быстро скатываются в холивар. Но, кажется, я нашел способ "вести разговоры о высоком без боли".

Мысль такая, если приземлить обсуждение на конкретную практическую задачу, то будет сильно проще понять, какой именно смысл вкладывает в слово "выразительность" собеседник. 

Задача

Мы делаем аддон для Confluence Cloud на основе Play Framework. Собственно, чем-то таким я каждый день занимаюсь в Stiltsoft.

Внутри приложения у нас есть несколько десятков контроллеров.

public class FeaturePageController extends Controller {
 
    public CompletionStage<Result> someAction1(Page page, Account account) {
        return completedFuture(ok());
    }
 
    public CompletionStage<Result> someAction2(Page page, Account account) {
        return completedFuture(ok());
    }
 
    public CompletionStage<Result> someAction3(Page page, Account account) {
        return completedFuture(ok());
    }
}

Page - это какая-то страница в Confluence

Account - это какой-то пользователь в Confluence

При обращении к контроллеру (вызов любого из его методов) надо проверить, имеет ли право пользователь account в принципе видеть страницу page. 

Сервис, выполняющий проверку, выглядит так

public interface PermissionService {
 
    CompletionStage<Boolean> canViewPage(ACHost acHost, Account account, Page page);
 
}

ACHost - это конкретный экземпляр Confluence, c которым взаимодействует наш сервис (сокращение от Atlassian Connect Host)

Сами параметры для canViewPage можно извлечь из запроса статическими методами

  • Optional<ACHost> getAcHost(Request request)

  • Optional<Account> getAccount(Request request)

  • Optional<Page> getPage(Request request) 

Надо интегрировать PermissionService в инфраструктуру Play Framework, чтобы не выписывать проверки руками каждый раз, а просто вешать аннотацию на нужный контроллер.

Исходный код для посмотреть/покрутить в IDE

Собственно, я вижу, как минимум, пять вариантов решения задачи. Давайте обсудим их в комментариях :)

Вариант первый. Простой, но суровый

Решаем задачу в лоб.

public class ActionV1 extends Action<RequireAccessToPage> {
 
    private PermissionService permissionService;
 
    @Inject
    public ActionV1(PermissionService permissionService) {
        this.permissionService = permissionService;
    }
 
    @Override
    public CompletionStage<Result> call(Request request) {
        Optional<ACHost> acHostOpt = getAcHost(request);
        Optional<Account> accountOpt = getAccount(request);
        Optional<Page> pageOpt = getPage(request);
 
        if (acHostOpt.isPresent() && accountOpt.isPresent() && pageOpt.isPresent()) {
            return permissionService.canViewPage(acHostOpt.get(), accountOpt.get(), pageOpt.get()).thenComposeAsync(canView -> {
                if (canView) {
                    return delegate.call(request);
                } else {
                    return completedFuture(forbidden());
                }
            });
        } else {
            return completedFuture(unauthorized());
        }
    }
}

Сильные стороны решения

  • чтобы понять код, достаточно знать стандартную библиотеку java 8

  • прямолинейный код: взяли, проверили, вызвали

Слабые стороны решения

  • три уровня вложенности, два из которых условия

  • визуально занимает много места, хотя ничего сложного не происходит

  • не идиоматичное использование Optional

Вариант второй. Идиоматический

Берем в руки Optional и начинаем "флэтмапать" на все деньги

public class ActionV2 extends Action<RequireAccessToPage> {
 
    private PermissionService permissionService;
 
    @Inject
    public ActionV2(PermissionService permissionService) {
        this.permissionService = permissionService;
    }
 
    @Override
    public CompletionStage<Result> call(Request request) {
        return getAcHost(request).flatMap(acHost ->
                getAccount(request).flatMap(account ->
                        getPage(request).map(page ->
                                permissionService.canViewPage(acHost, account, page))))
                .map(permission -> permission.thenComposeAsync(canView -> canView ? delegate.call(request) : completedFuture(forbidden())))
                .orElse(completedFuture(unauthorized()));
    }
}

Сильные стороны решения

  • минимум кода

  • последовательный код, всего одно явное условие

  • ленивость, если какое-то из значений не будет найдено, поиск следующего даже не начнется

Слабые стороны решения

Вариант третий. Хипстерский

Вооружаемся терминологией в стиле "моноид в категории эндофункторов" и пишем

public class ActionV3 extends Action<RequireAccessToPage> {
 
    private PermissionService permissionService;
 
    @Inject
    public ActionV3(PermissionService permissionService) {
        this.permissionService = permissionService;
    }
 
    @Override
    public CompletionStage<Result> call(Request request) {
        Function3<ACHost, Account, Page, CompletionStage<Boolean>> canViewPage = permissionService::canViewPage;
 
        return Optional.of(canViewPage.curried())
                .flatMap(acHostFn -> getAcHost(request).map(acHostFn))
                .flatMap(accountFn -> getAccount(request).map(accountFn))
                .flatMap(pageFn -> getPage(request).map(pageFn))
                .map(permission -> permission.thenComposeAsync(canView -> canView ? delegate.call(request) : completedFuture(forbidden())))
                .orElse(completedFuture(unauthorized()));
    }
}

Сильные стороны решения

  • последовательный пайплайн

  • ленивость

Слабые стороны решения

Function3 вместе с методом curried предоставлена Vavr

Вариант четвертый. Акценты на главном

Код пишется для людей и есть вероятность, что потом его будут читать и править. Поэтому, пытаемся явным образом обособить главное, чтоб было понятно, ради чего вообще это написано все.

public class ActionV4 extends Action<RequireAccessToPage> {
 
    private PermissionService permissionService;
 
    @Inject
    public ActionV4(PermissionService permissionService) {
        this.permissionService = permissionService;
    }
 
    @Override
    public CompletionStage<Result> call(Request request) {
        return hasPermission(request, permissionService::canViewPage);
    }
 
    private CompletionStage<Result> hasPermission(Request request, Function3<ACHost, Account, Page, CompletionStage<Boolean>> canViewPage) {
        return permissionRequest(request)
                .map(canViewPage.tupled())
                .map(permission -> permission.thenComposeAsync(canView -> canView ? delegate.call(request) : completedFuture(forbidden())))
                .orElse(completedFuture(unauthorized()));
    }
 
    private Optional<Tuple3<ACHost, Account, Page>> permissionRequest(Request request) {
        return getAcHost(request).flatMap(acHost ->
                getAccount(request).flatMap(accountId ->
                        getPage(request).map(pageId ->
                                Tuple.of(acHost, accountId, pageId))
                ));
    }
}

Сильные стороны решения

  • четкое выделение "бизнес-логики"

  • ленивость

Слабые стороны решения

  • нужен дополнительный код, для обеспечения инфраструктурных вещей

Вариант пятый. Прагматичный

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

public class ActionV5 extends Action<RequireAccessToPage> {
 
    private PermissionService permissionService;
 
    @Inject
    public ActionV5(PermissionService permissionService) {
        this.permissionService = permissionService;
    }
 
    @Override
    public CompletionStage<Result> call(Request request) {
        ACHost acHost = getAcHost(request).orElse(null);
        Account account = getAccount(request).orElse(null);
        Page page = getPage(request).orElse(null);
 
        if (anyNull(acHost, account, page)) {
            return completedFuture(unauthorized());
        }
 
        return permissionService.canViewPage(acHost, account, page)
                .thenComposeAsync(canView -> canView ? delegate.call(request) : completedFuture(forbidden()));
    }
}

Сильные стороны решения

  • минимум кода при максимуме выразительности

Слабые стороны решения

  • людей может смущать идиома early return 

  • есть вопросики к использованию Optional 

anyNull предоставлен Apache Commons

Опрос

Читателю я предлагаю ответить на вопрос: какой вариант нравится больше остальных, а в комментариях рассказать почему (или написать свой если все варианты плохи)

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


  1. brukva
    28.01.2022 12:45
    #24000783
    +4

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


    1. e_Hector Автор
      29.01.2022 10:43
      #24004153
      +1

      А как вы определяете читаемость?

      Гипотетически, если бы мне действительно надо было проходить ревью у вас, как бы я мог понять, достаточно ли читаемо написал?


      1. martin_wanderer
        30.01.2022 11:50
        #24007951
        +1

        Читаемость - понятие заведомо субъективное. То есть то, что читаемо дня одного, совершенно нечитаемо для другого. Более того, в процессе ревью исходно нечтаемымый код становится читаемым, когда ты таки понимаешь, что хотел сказать автор.

        Поэтому в некоторых коллективах MR принимается по факту наличия одобрения от нескольких ревьюеров


        1. e_Hector Автор
          31.01.2022 09:20
          #24011237

          По-моему, сейчас вы смешиваете "понятность" и "читаемость"

          Понятность субъективна, потому что зависит от опыта читающего.

          Читаемость же, вполне себе объективная характеристика

          • на запихиваем 100500 действий в одну строку

          • ограничиваем предельный размер строки

          • отделяем пустыми строками важные части

          • предпочитаем короткие имена вместо ОоченьПодробныхНоСлишкомПримитивных

          • придерживаемся единообразия в форматировании


          1. elishagg
            31.01.2022 10:35
            #24011507

            У вас "очипятка" перед 100500. И оченьДлинныеИмена лучше верблюдом писать, там читаемей.


  1. MentalBlood
    28.01.2022 12:59
    #24000841
    +4

    Лучший код — это ненаписанный код

    Поэтому чем меньше кода, тем больше лучшего кода


    1. klounader
      31.01.2022 10:39
      #24011527

      Ассемблер рулит


  1. quaer
    28.01.2022 20:34
    #24002711

    А вы не сравнивали скорость работы и потребление памяти для подходов со stream и при использовании циклов и кодирования ручками?


    1. e_Hector Автор
      29.01.2022 10:23
      #24004059

      Нет, никакой "технической оценки" вариантов не делали.


  1. Bakuard
    29.01.2022 07:58
    #24003767

    Прежде чем пытаться ответить на вопрос о том, какой код лучше, следует определить критерии оценки кода.


    1. e_Hector Автор
      29.01.2022 10:41
      #24004145

      Тут есть ловушка

      Дело в том, что "критерии лучшести" часто выражаются абстрактными словами.

      Допустим, мы считаем, что хороший код должен быть выразительным и поддерживаемым (и работать правильно, само собой)

      А что такое выразительность и поддерживаемость?


      1. Bakuard
        29.01.2022 13:11
        #24004723
        +1

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

        Предложу свой вариант: хороший код это такой код, который позволяет как можно быстрее вносить изменения сохраняя при этом минимально необходимый уровень качества разрабатываемого ПО. Второй критерий (качество ПО) можно определить как соответствие реального поведения программы ожидаемому поведению описанному в ТЗ. Используя эти два критерия можно точно определить уровень качества кода, причем выразить его числом.


        1. e_Hector Автор
          29.01.2022 13:54
          #24004915

          На мой взгляд, ваши критерии

          • позволяет как можно быстрее вносить изменения сохраняя при этом минимально необходимый уровень качества

          • соответствие реального поведения программы ожидаемому поведению описанному в ТЗ

          все равно довольно абстрактны.

          Сейчас я не понимаю как их можно свести к числам.

          Если вам не трудно, можете провести оценку пары вариантов, например, первого и пятого по своей схеме?

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

          Если нужны какие-то уточнения по ТЗ, или другого плана, то я готов отвечать на вопросы.


          1. Bakuard
            29.01.2022 15:28
            #24005277
            +1

            "Если вам не трудно, можете провести оценку пары вариантов, например, первого и пятого по своей схеме? " - не трудно) Мне самому интересна эта тема.

            "позволяет как можно быстрее вносить изменения сохраняя при этом минимально необходимый уровень качества" - здесь два критерия, а не один:
            1. позволяет вносить изменения как можно быстрее
            2. соответствие реального поведения программы ожидаемому поведению описанному в ТЗ

            Первый критерий оценивается просто: сколько времени у вас ушло на написание каждого варианта, сколько времени уйдет на внесение изменений, сколько времени ушло на отладку, и главное - сколько времени ушло на все суммарно. Заметьте, тут в первую очередь оцениваются именно Ваши затраты времени.
            Лично у меня пятый вариант занял меньше всего времени, чтобы разобраться в нем. Поэтому с точки зрения этого критерия, ИМХО, он лучший.

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


            1. e_Hector Автор
              29.01.2022 19:20
              #24005963

              не трудно) Мне самому интересна эта тема.

              Отлично же :) Тогда давайте я попробую применить ваши критерии.

              Первый вариант я написал за 5 минут.
              Чтобы написать пятый вариант, понадобилось 10 минут.

              Цифры условные, никаких замеров я не делал, но по ощущениям на пятый вариант потрачено в два раза больше времени.

              Технически, я сначала написал вариант 1, а потом потратил еще столько же времени, чтобы его улучшить и, в итоге, получился вариант 5.

              Сколько времени уйдет на внесение изменений в будущем не понятно. Ведь мы не знаем какого рода это будут изменения и кто их будет вносить.

              В силу простоты задачи какой-то явно заметной отладки не получилось.

              На решение написано 5 тестов:

              • 3 теста на случай когда не передали какой-то из параметров

              • 1 тест когда пользователь не имеет доступа и получает отлуп

              • 1 тест когда все хорошо и доступ есть

              Оба варианта успешно проходят все тесты.

              Как понять какой вариант лучше? Первый, потому что потребовал меньше времени?

              Кстати, если я не знаю сколько времени потрачено на написание кода, например, смотрю на код опенсорсной библиотеки, то получается в рамках этой схемы качество оценить нельзя?


              1. Bakuard
                31.01.2022 11:17
                #24011721

                "Как понять какой вариант лучше? Первый, потому что потребовал меньше времени? " - выходит, что первый вариант самый лучший.

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

                Если изменятся или добавятся требования, то оценка уточняется (к результату старой оценки добавляются результаты новой оценки).


                1. Koval97
                  31.01.2022 12:11
                  #24012057

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

                  Для того, чтобы понять что и куда двигается, достаточно понять непосредственно что вы делаете и для кого, а точнее для какого пользователя вы это делаете.

                  Бывает, что софт проваливается только из-за того, что держал пользователя за идиота. Аналогично, когда функционал перенасыщен настолько, что основные функции софт уже выполнять не может.

                  И какого-то особого опыта там не требуется. Его вообщем и не определишь. Погружение в проект давно известная дилемма в управление.

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

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

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


  1. someoneinthebox
    29.01.2022 10:24
    #24004065

    Если это работает - это не глупо. А оптимизировать код можно до бесконечности. Если ваш код будут читать другие программисты - пишите понятно, а не академически. Я за такой подход.


    1. e_Hector Автор
      29.01.2022 10:32
      #24004103

      Тут есть забавный нюанс.

      Характеристики "понятно" и "академически", сами по себе, довольно абстрактны и каждый человек вкладывает в них какой-то свой смысл.

      Когда я обсуждал статью с коллегами внутри компании, то получалась такая ситуация
      * "простым" считали либо первый, либо пятый
      * "академическим" считали либо второй, либо третий

      Вот лично для вас, на пальцах, что значит "просто", а что "академически"?


      1. someoneinthebox
        29.01.2022 11:24
        #24004329

        Для меня "академический" - максимально эффективный и, как правило, короткий код. Но часто он бывает непонятным, вернее, времени на его понимание другим разработчиком уходит больше. Для больших команд лучше писать менее эффективно, но более понятно. Эдакий консенсус. Сложно упрощать, но в крайности тоже врадать не стоит.

        А "простой" - это тот код, который читается без запинок в понимании. Но это не всегда равно эффективный, конечно же.


        1. e_Hector Автор
          29.01.2022 14:00
          #24004945

          До вашего пояснения я думал что между "понятно" и "академически" существует дихотомия.

          Теперь я уже не уверен. В вашей "системе ценностей" код может быть одновременно и "понятным" и "академическим" (варианты 1 и 5), или я что-то понял не так?

          Если вам не трудно, могли бы в классифицировать примеры из статьи по системе "понятно"/"академически" с комментариями почему именно так?


      1. Koval97
        31.01.2022 06:26
        #24010955

        Характеристики "понятно" и "академически", сами по себе, довольно абстрактны и каждый человек вкладывает в них какой-то свой смысл.

        Как всегда говорят классики: "Программист в такой момент руководствуется только своим чувством прекрасного".

        Не важно как пишет программист: разляписто или элегантно, в одну строку или со структурой, применяя хакерские приемы или не применяя. Извечно все спрашивают только два вопроса: Что вы делаете? Зачем вы это делаете?

        Ваше "Как это делают?" скорее риторический вопрос, чем математическое уравнение. Именно это и подчеркивалось в грубой, но меткой формулировке "Лучший код - это ненаписанный код", говоря о том, что часто лучшее решение там, где не требуются лишние проверки и формальности.


        1. e_Hector Автор
          31.01.2022 09:26
          #24011249

          Я не понял, что вы хотели сказать :(

          Не важно, как написан код, главное чтоб задача решалась?

          Вообще не надо писать код, надо находить какие-то другие пути?


          1. Koval97
            31.01.2022 10:02
            #24011351

            Не важно, как написан код, главное чтоб задача решалась?

            А в чем тогда цель code review? Внушать программистам волю team lead-а, которому не хватает ума, чтобы набраться авторитета, а назначить из команды мозгов уже менеджерам не хватило? Заражать предрассудками о неком "идеальном чистом коде", попутно вырабатавыя лишь комплекс неполноценности?

            Именно такие вопросы приходят на ум, после всего что я тут прочитал.

            А если вы хотите узнать о настоящей деловой коммуникации, то советую почитать.

            Вообще не надо писать код, надо находить какие-то другие пути?

            Нет, я ответ дал в последнем абзаце. Не вежливо, знаете, отвечать человеку не дочитав его сообщения.

            Я так скажу. Вообще в программирование нет понятия "эстетики кода". Классики ещё до вашего рождения здесь всё расписали. Мы все когда-то проходили через этап комплекса перфекционизма, когда только начинали программировать, но с опытом приходит понимание и творческое осознание. Именно оно, к слову, отличает начинающего от специалиста, а не какие-то там железные принципы и тем более предрассудки. Михаил Флёнов в своих книгах "Программирование глазами хакера" отмечал это разницей между "хакером" и "крекером" (от англ. crack - "взломать") в самой первой главе.


  1. arTk_ev
    30.01.2022 10:15
    #24007655
    -1

    Каждый из вариантов - говнокод

    1. Нейминг

    2. Применение ООП не в том месте и ещё и с логикой

    3. Перепутаны зависимости.

    4. Сущность делает не свое дело и варинципе не нужна.

    5. Нарушение принципоsolid

    Не используйте ООП внутри архитектуры, тем более наследование

    Нейминг сущностей важнее кода.

    Логика и данные должны быть отделены.

    Минимизировать зависимости.

    Сущность использует сеть для запроса, следовательно она должна обратиться к сервису запросов. Значит у нее очень большая власть. Но как только вы правильно назовёте класс, то станет очевидным что эта сущность в принципе не нужна. Ваша тезис оказался верным.


    1. arTk_ev
      30.01.2022 11:16
      #24007837
      -1

      Мы делаем аддон для Confluence Cloud на основе Play Framework

      Не заметил. Тогда ооп применимо в таких местах. Архитектура фреймворков отличается

      Но все равно Action - объект который не вписывается в иерархию.

      У вас уже есть PermissionService. Это он дожен вызывать запросы, и в нем должна быть бизнес-логика. Хотя непонятно назначение PermissionService, разве запросов на доступ будет много? Но очевидно что он зависит от более общего RequestService. ViewModel страницы вызывает уже IRequestService и заполняет Model, про сервер и запросы она ничего не знает.

      Сущность Action - не имеет смысла. Количество запросов будет все время расти и изменятся, наследование этому помешает. Тут можно использовать паттерн стратегия, когда увеличется количество.


    1. 0xf331d34d
      30.01.2022 15:37
      #24008779

      А какой принцип солид нарушен в 5 пункте?


  1. vip_delete
    30.01.2022 14:27
    #24008479
    +4

    Вариант 1

    • Используется Optional.get значит уже что-то неправильное происходит, либо вам не нужны Optional. 1 WTF.

    • Вложенные IF когда это можно избежать. 1 WTF

    • thenComposeAsync да еще и в commonPool! Тут вопрос насколько автор действительно хочет следующий stage запускать всегда в другом потоке из commonPool, а не в том же потоке в каком логика canViewPage. permissionService тоже ведь асинхронный и тоже что-то запускает в другом потоке. Так зачем эти прыжки из потока в поток? Тут 3 WTF.

    Вариант 2

    • permissionService.canViewPage возвращает Optional<CompletionStage<Boolean>> что-ли? Может и не возвратить stage? Тут ошибка дизайна какая-то ради flatMap. Сразу 10 WTF

    Вариант 3

    • Function3 - ошибка дизайна, надо создать класс с нормальным именем, вроде SecurityRequest, в котором будет 3 поля и он будет передаваться в canViewPage. 10 WTF

    Вариант 4

    • Tuple - 10 WTF, то же самое. Никаких Tuple, если это не математические вычисления, быть не может.

    Вариант 5

    • orElse(null) как раз и говорит, что Optional, по крайней мере тут в API, не нужен. На уровне синтаксиса java не смогли сделать как в Kotlin, дали костыль - Optional.

    Еще нет логов.

    Там static import на CompletableFuture? Очень сложно ревьюить. Имхо это оправдано только в тестах, где очень много Mockito и все понимают, что это static import.

    public CompletionStage<Result> call(Request request) {
    ACHost acHost = getAcHost(request);
    if (acHost == null) {
        log.debug("Request(id={}).acHost is empty: unauthorized", request.getId());
        return CompletableFuture.completedFuture(unauthorized());
    }

    Account account = getAccount(request);
    if (account == null) {
        log.debug("Request(id={}).account is empty: unauthorized", request.getId());
        return CompletableFuture.completedFuture(unauthorized());
    }

    Page page = getPage(request);
    if (page == null) {
        log.debug("Request(id={}).page is empty: unauthorized", request.getId());
        return CompletableFuture.completedFuture(unauthorized());
    }

    return startAsync(start -> start
            .thenCompose(unused -> {
                log.debug("Request(id={}) processing: acHost={}, account={}, page={}",
                        request.getId(),
                        acHost,
                        account,
                        page
                );
                return permissionService.canViewPage(acHost, account, page);
            }).thenCompose(canView -> {
                if (canView) {
                    log.debug("Request(id={}) authorized", request.getId());
                    return delegate.call(request);
                } else {
                    log.debug("Request(id={}) forbidden", request.getId());
                    return CompletableFuture.completedFuture(forbidden());
                }
            })
    );
}

    startAsync - это такая штука которая лямбду всегда зпускает в другом потоке. Вначале создаем цепочку stage, потом запускаем в каком-нибудь execution service.

    public CompletionStage<Result> startAsync(Function<CompletionStage<Void>, CompletionStage<Result>> closure) {
    CompletableFuture<Void> start = new CompletableFuture<>();
    CompletionStage<Result> cs = closure.apply(start);
    start.completeAsync(() -> null, es);
    return cs;
}


    1. e_Hector Автор
      31.01.2022 09:59
      #24011341

      Вот ради таких комментариев я эту статью и написал. Отдельное спасибо за свой вариант кода.

      Правильно ли я понимаю, что "победил" вариант 5 со счетом 1 WTF?

      И еще момент.

      Как предлагается решать вопросы субъективности WTF-метрики?
      Лично знаю людей, которые не считают Optional.get проблемой с мотивацией вида "раз метод существует и не помечен устаревшим, то можно пользоваться"


      1. vip_delete
        31.01.2022 10:35
        #24011505

        Да, вариант 5 самый читабельный.

        Код пишется 1 раз, а читается много, поэтому важно, чтобы написано было так, чтобы его легко было читать, отлаживать, тестировать, расширять и переиспользовать. Для этого придумали Naming Convention, Code Style, комментарии, логирование, а так же подходы solid, dry, kiss, yagni, ...

        По поводу Option.get, если делать правильно, то get вызывать не нужно никогда, но код превращается в функциональный, и кто-то считает это хуже Option.get, хотя я так не считаю. Если с Option делать и не писать все в 1 строку, то получится вполне читабельный код на функциональном языке.

        public CompletionStage<Result> call(Request request) {
    return getAcHost(request).map(acHost -> {
        // acHost is set
        return getAccount(request).map(account -> {
            // account is set
            return getPage(request).map(page -> {
                // page is set
                return permissionService.canViewPage(acHost, account, page).thenCompose(canView -> {
                    if (canView) {
                        // authorized
                        return delegate.call(request);
                    } else {
                        return completedFuture(forbidden());
                    }
                });
                // page is not set
            }).orElse(completedFuture(unauthorized()));
            // account is not set
        }).orElse(completedFuture(unauthorized()));
        // acHost is not set
    }).orElse(completedFuture(unauthorized()));
}

        От субьективности WTFs/minute никуда не уйти, даже оценки в школе мало коррелируют со знаниями. Есть книга Clean Code - одна из обязательных книг для программиста.


  1. Whiteha
    30.01.2022 14:47
    #24008581
    +1

    А где вариант обернуть permission service в сущность вроде RequestPermissions, которая по реквесту будет говорить да/нет, чтобы не дублировать логику во всех экшонах. Почему бы не свести использование этого RequestPermission в одно место, в базовом классе например, или через композицию в виде класса CheckedAction, получающего RequestPermission и Action.


    1. e_Hector Автор
      31.01.2022 09:45
      #24011295
      +1

      Так ведь именно это и сделано.

      PermissionService используется только в одном месте, в Action

      Этот Action силами фреймворка вызывется прежде чем пустить запрос к контроллеру

      Контроллеры на которых такая проверка нужна просто помечаются аннотацией RequireAccessToPage

      Или я неправильно вас понял?


  1. Koval97
    31.01.2022 06:09
    #24010947

    Вариант шесть. Автор начинайте с определения. Договорившись об значение терминов, а ешё лучше приведя академическую формулировку из первоисточника, вы решите 90% проблемы.


    1. e_Hector Автор
      31.01.2022 09:39
      #24011273

      Так я и пытаюсь договориться об определении :)

      Есть задача и есть 5 вариантов решения (ситуация в статье)

      Нужно взять самый качественный/лучший/хороший вариант.

      И, внезапно, выясняется, что 3 человека выбирая самый качественный/лучший/хороший вариант взяли разные варианты.

      Очевидно, что понятие "качественный" у каждого из них свое. Но как понять, из чего оно складывается?

      Я решил поспрашивать по-больше людей, чтоб оценить критерии качества в крупную клетку.

      Например, что вы думаете о предложенных вариантах?
      Какой из них предпочли бы видеть у себя в коде?
      Какой видеть не хотели бы?


      1. Koval97
        31.01.2022 10:17
        #24011437

        Есть задача и есть 5 вариантов решения (ситуация в статье)

        А что если скажу, что все ваши 5 вариантов - это полное гавное, понятное только вам и адептам Java, но совершенно не понятное никому-либо прочитавшему это?

        Вы хотя бы знаете что такое комментарии в исходнике?!

        Нужно взять самый качественный/лучший/хороший вариант.

        И, внезапно, выясняется, что 3 человека выбирая самый качественный/лучший/хороший вариант взяли разные варианты.

        Значит более опытные, чем их коллеги оказались. Или менее закомплексованные, что более вероятно.

        На месте ваших коллег я бы сразу спросил: А нахрена там эта проверка? Лучше тогда сделать обработку ошибки. И засунул функцию в класс-предок, так как у меня одна функциональность уходит в три разных класса.


      1. Koval97
        31.01.2022 10:26
        #24011471

        Очевидно, что понятие "качественный" у каждого из них свое. Но как понять, из чего оно складывается?

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

        В технике вообще часто приходится идти на компромиссы, чтобы добится лучшего результата. В вашем случае, если хотите добится именно "эффективности", то оно решается средствами отладки, а не правилами кодонаписания.