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

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

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

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

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

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

Задача

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

Опрос

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