Да что это такое, ваше качество кода?

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

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

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

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

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

Задача

Мы делаем аддон для 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()));     } }

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

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

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

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

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

• callback hell

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

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

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

Опрос

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