Лучший код — это ненаписанный код
Салют, коллеги.
Лично я, очень люблю поговорить про качество, поддерживаемость и выразительность кода (эти умные слова, часто звучат на код ревью)
К сожалению, такие разговоры часто и быстро скатываются в холивар. Но, кажется, я нашел способ "вести разговоры о высоком без боли".
Мысль такая, если приземлить обсуждение на конкретную практическую задачу, то будет сильно проще понять, какой именно смысл вкладывает в слово "выразительность" собеседник.
Задача
Мы делаем аддон для 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
Опрос
Читателю я предлагаю ответить на вопрос: какой вариант нравится больше остальных, а в комментариях рассказать почему (или написать свой если все варианты плохи)
