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

Моя цель - предложение широкого ассортимента товаров и услуг на постоянно высоком качестве обслуживания по самым выгодным ценам.

Прежде чем перейти к статье, хочу вам представить, экономическую онлайн игру Brave Knights, в которой вы можете играть и зарабатывать. Регистируйтесь, играйте и зарабатывайте!

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

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

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

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

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

Задача

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

Опрос

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

Только зарегистрированные пользователи могут участвовать в опросе. Войдите, пожалуйста.
Какой вариант, по вашему, больше всего подходит под определение «качественный код»?
50% Вариант первый. Простой, но суровый 2
0% Вариант второй. Идиоматический 0
0% Вариант третий. Хипстерский 0
0% Вариант четвертый. Акценты на главном 0
50% Вариант пятый. Прагматичный 2
Проголосовали 4 пользователя. Воздержавшихся нет.
Источник: https://habr.com/ru/post/648311/


Интересные статьи

Интересные статьи

Качество видео-конференц-связи зависит от целого ряда факторов, о которых вы могли не подозревать. В этой статье приведем рассказ разработчиков коммуникационной платформы 3СХ о том, как с...
Эта публикация написана после неоднократных обращений как клиентов, так и (к горести моей) партнеров. Темы обращений были разные, но причиной в итоге оказывался один и тот же сценарий, реализу...
Прим. перев.: С Kubernetes-энтузиастами из проекта Garden мы познакомились на недавнем мероприятии KubeCon Europe 2019, где они произвели на нас приятное впечатление. Этот их материал, написанный...
В статье была затронута тема тональности и голоса продукта. Понятие в наших краях не слишком-то и обсуждаемо, практически не используется осознанно. И, если быть честными, голос и тон обычно вход...
Эта статья посвящена одному из способов сделать в 1с-Битрикс форму в всплывающем окне. Достоинства метода: - можно использовать любые формы 1с-Битрикс, которые выводятся компонентом. Например, добавле...