Программный код пишется сверху вниз. В том же порядке читаются и выполняются инструкции. Это логично и все давно к этому привыкли. В некоторых случаях можно поменять порядок операций. Но иногда очерёдность вызовов функций важна, хотя синтаксически это неочевидно. Получается, что код выглядит рабочим, даже после перестановки вызовов, а результат оказывается неожиданным.
Однажды подобный код попался мне на глаза.
Проблема
Однажды, ковыряясь в чужом коде в совместном проекте, а обнаружил функцию наподобие:
public Product fill(Product product, Images images, Prices prices, Availabilities availabilities){
priceFiller.fill(product, prices); //do not move this line below availabilityFiller call, availabilities require prices
availabilityFiller.fill(product, availabilities);
imageFiller.fill(product, images);
return product;
}Конечно сразу в глаза бросается не самый "элегантный" стиль: классы хранят данные (POJO), функции изменяют входящие объекты...
В целом вроде бы ничего. У нас есть объект, в котором не хватает данных, и есть сами данные, пришедшие из других источников (сервисов), которые мы теперь в этот объект и поместим, чтобы сделать его полноценным.
Но есть и некоторые нюансы.
- Мне не нравится, что ни одна функция не написана в стиле ФП и изменяет объект, переданный как аргумент.
- Но допустим, что это было сделано, чтобы сократить время обработки и количество создаваемых объектов.
- Только комментарий в коде говорит о том, что последовательность вызовов важна, и при встраивании нового
Fillerнадо быть осторожным
- А ведь количество людей, работающих над проектом больше 1 и не все знают об этой хитрости. Особенно новые люди в команде (не обязательно на предприятии).
Последний пункт меня особенно насторожил. Ведь API построена так, что мы не знаем, что изменилось в объекте, после вызова данной функции. Например у нас до и после есть метод Product::getImages, который до вызова функции fill выдаст пустой список, а после список с картинками к нашему продукту.
С самими Filler дела обстоят ещё хуже. AvailabilityFiller никак не даёт понять, что ожидает, что информация о цене товара уже заложена в передаваемых объект.
И вот я задумался о том, как бы я мог оградить своих коллег от ошибочного использования функций.
Предложенные решения
Сперва, я решил обсудить данный случай со своими коллегами. К сожалению, все предложенные ими решения, не показались мне правильным подходом.
RuntimeException
Одним из предложенных вариантов был: а ты в AvailabilityFiller впиши в начале функции Objects.requireNonNull(product.getPrices) и тогда любой программист во время локальных тестов уже получит ошибку.
- но цены и правда может не быть, если сервис был недоступен или ещё какая ошибка, тогда товар должен получить статус "нет в наличии". Придётся приписывать всякие флаги или прочее, чтобы отличить "нет данных" от "даже не запрашивали".
- Если же бросить исключение в самом
getPrices, тогда мы создадим те же проблемы, что и современная Java со списками
- Допустим в функцию передаётся List, предлагающий в своей API метод get… Я знаю, что не надо менять передаваемые объекты, а создавать новые. Но суть в том, что API нам такой метод предлагает, но в рантайме может вылететь ошибка, если это неизменяемый список, как например полученный из Collectors.toList()
- Если же бросить исключение в самом
- если
AvailabilityFillerбудет кем-то использован в другом месте, то программист написавший вызов, не сразу поймёт, в чём проблема. Только после запуска и Дебага. Потом ему ещё придётся разбираться в коде, чтобы понять, откуда взять данные.
Test
"А ты напиши тест, который будет ломаться, если поменять очерёдность вызовов". т.е. если все Filler будут возвращать "новый" продукт, получится что-то наподобие:
given(priceFillerMock.fill(eq(productMock), any())).willReturn(productWithPricesMock);
given(availabilityFillerMock.fill(eq(productMockWithPrices), any())).willReturn(productMockWithAvailabilities);
given(imageFillerMock.fill(eq(productMockWithAvailabilities), any())).willReturn(productMockWithImages);
var result = productFiller.fill(productMock, p1, p2, p3);
assertThat("unexpected return value", result, is(productMockWithImages));- не люблю тесты, которые настолько "White-Box"
- Ломается при каждом новом
Filler - Ломается при смене очередности независимых вызовов
- Опять таки не решает проблему переиспользования самого
AvailabilityFiller
Собственные попытки решения проблемы
Идея
Думаю, что вы уже догадались, что я бы хотел решить проблему на уровне компиляции. Ну зачем мне, спрашивается, компилируемый язык со строгой типизацией, если я не могу предотвратить ошибку.
И я задумался, принадлежат ли объект без дополнительных данных и "расширенный" объект к одному и тому же классу?
Разве не будет правильным описать отдельными классами или интерфейсами различные возможные состояния объекта?
Таким образом моя идея состояла в следующем:
public Product fill(<? extends BaseProduct> product, Images images, Prices prices, Availabilities availabilities){
var p1 = priceFiller.fill(product, prices);
var p2 = availabilityFiller.fill(p1, availabilities);
return imageFiller.fill(p2, images);
}
PriceFiller
public ProductWithPrices fill(<? extends BaseProduct> product, Prices prices)
AvailabilityFiller
public ProductWithAvailabilities fill(<? extends ProductWithPrices> product, Prices prices)
или
public <BaseProduct & PriceAware & AvailabilityAware> fill(<? extends BaseProduct & PriceAware> product, Prices prices)Т.е. задаваемый изначально продукт является инстансом класса отличного от возвращаемого, чем уже показаны изменения данных.
Filler'ы же в своих API точно указывают, какие данные им нужны и что они возвращают.
Таким образом можно предотвратить неправильную очередность вызовов.
Реализация
Как такое воплотить в реальность в Java? (Напомню, что наследование от нескольких классов в Java невозможно.)
Сложность добавляют независимые операции. Например, картинки могут быть добавлены и до и после добавления цен, а так же в самом конце функции.
Тогда могут быть
class ProductWithImages extends BaseProduct implements ImageAware{}
class ProductWithImagesAndPrices extends BaseProduct implements ImageAware, PriceAware{}
class Product extends BaseProduct implements ImageAware, PriceAware, AvailabilityAware{}Как это всё описать?
Создавать адаптеры?
public ProductWithImagesAndPrices(<? extends BaseProduct & PriceAware> base){
this.base = base;
this.images = Collections.emptyList();
}
public long getId(){
return this.base.getId();
}
public Price getPrice(){
return this.base.getPrice();
}
public List<Image> getImages(){
return this.images;
}Копировать данные/ссылки?
public ProductWithImagesAndPrices(<? extends BaseProduct & PriceAware> base){
this.id = base.getId();
this.prices = base.getPrices();
this.images = Collections.emptyList();
}
public List<Image> getImages(){
return this.images;
}Как уже заметно, всё сводится просто к огромнейшему количеству кода. И это при том, что в примере оставил только 3 вида входных данных. В реальном мире их может быть намого больше.
Получается, что затраты на написание и поддержку такого кода себя не оправдывают, хотя сама идея разделения состояния на отдельные классы мне показалась очень привлекательной.
Отступление
Если посмотреть на другие языки, то где-то эту проблему решить проще, а где-то нет.
Например в Go можно прописать ссылку на расширяемый класс без "копирования" или "перегрузки" методов. Но речь не о Go
Ещё одно отступление
Пока писал эту статью, пришло в голову ещё одно возможное решение с Proxy, позвалающее прописывать только новые методы, но требующее иерархии интерфейсов. В общем страшное, сердитое и не подходящее. Если вдруг кому-то интересно:public class Application {
public static void main(String[] args) {
var baseProduct = new BaseProductProxy().create(new BaseProductImpl(100L));
var productWithPrices = fillPrices(baseProduct, BigDecimal.TEN);
var productWithAvailabilities = fillAvailabilities(productWithPrices, "available");
var productWithImages = fillImages(productWithAvailabilities, List.of("url1, url2"));
var product = productWithImages;
System.out.println(product.getId());
System.out.println(product.getPrice());
System.out.println(product.getAvailability());
System.out.println(product.getImages());
}
static <T extends BaseProduct> ImageAware fillImages(T base, List<String> images) {
return (ImageAware) Proxy.newProxyInstance(base.getClass().getClassLoader(), new Class[]{ImageAware.class, BaseProduct.class},
new MyInvocationHandler<>(base, new ImageAware() {
@Override
public List<String> getImages() {
return images;
}
}));
}
static <T extends BaseProduct> PriceAware fillPrices(T base, BigDecimal price) {
return (PriceAware) Proxy.newProxyInstance(base.getClass().getClassLoader(), new Class[]{PriceAware.class},
new MyInvocationHandler<>(base, new PriceAware() {
@Override
public BigDecimal getPrice() {
return price;
}
}));
}
static AvailabilityAware fillAvailabilities(PriceAware base, String availability) {
return (AvailabilityAware) Proxy.newProxyInstance(base.getClass().getClassLoader(), new Class[]{AvailabilityAware.class},
new MyInvocationHandler<>(base, new AvailabilityAware() {
@Override
public String getAvailability() {
return base.getPrice().intValue() > 0 ? availability : "sold out";
}
}));
}
static class BaseProductImpl implements BaseProduct {
private final long id;
BaseProductImpl(long id) {
this.id = id;
}
@Override
public long getId() {
return id;
}
}
static class BaseProductProxy {
BaseProduct create(BaseProduct base) {
return (BaseProduct) Proxy.newProxyInstance(this.getClass().getClassLoader(), new Class[]{BaseProduct.class}, new MyInvocationHandler<>(base, base));
}
}
public interface BaseProduct {
default long getId() {
return -1L;
}
}
public interface PriceAware extends BaseProduct {
default BigDecimal getPrice() {
return BigDecimal.ZERO;
}
}
public interface AvailabilityAware extends PriceAware {
default String getAvailability() {
return "sold out";
}
}
public interface ImageAware extends AvailabilityAware {
default List<String> getImages() {
return Collections.emptyList();
}
}
static class MyInvocationHandler<T extends BaseProduct, U extends BaseProduct> implements InvocationHandler {
private final U additional;
private final T base;
MyInvocationHandler(T base, U additional) {
this.additional = additional;
this.base = base;
}
@Override
public Object invoke(Object proxy, Method method, Object[] args) throws Throwable {
if (Arrays.stream(additional.getClass().getInterfaces()).anyMatch(i -> i == method.getDeclaringClass())) {
return method.invoke(additional, args);
}
var baseMethod = Arrays.stream(base.getClass().getMethods()).filter(m -> m.getName().equals(method.getName())).findFirst();
if (baseMethod.isPresent()) {
return baseMethod.get().invoke(base, args);
}
throw new NoSuchMethodException(method.getName());
}
}
}Вывод
Что же получается? С одной стороны интересный подход, применяющий к "объекту" в разных состояниях отдельные классы и гарантирующий этим предотвращение ошибок, вызванных неправильной последовательностью вызовов методов, изменяющих этот объект.
С другой стороны, такой подход заставляет писать столько кода, что от него сразу хочется отказаться. Нагромождение интерфейсов и классов только мешает разобраться в проекте.
В другом своем проекте я всё же попробовал использовать такой подход. И сначала на уровне интерфейсов всё было просто отлично. Я написал функции:
<T extends Foo> List<T> firstStep(List<T> ts){}
<T extends Foo & Bar> List<T> nStep(List<T> ts){}
<T extends Foo> List<T> finalStep(List<T> ts){}Указав тем самым, что определённый шаг обработки данных требует дополнительной информации, которая не нужна ни в начале обратобки, ни в её конце.
Испльзуя mock'и, я сумел протестировать код. Но когда дело дошло до реализации и колличество данных и разных источников начало расти, я быстро сдался и переделал всё под "обычный" вид. Всё работает и никто не жалуется. Получается, что работоспособность и простота кода побеждают над "предотвращением" ошибок, а проследить за правильной последовательностью вызовов можно и вручную, пусть даже ошибка проявит себя только на этапе ручного тестирования.
Может, если бы я сделал шаг назад и посмотрел бы на код с другой стороны, у меня появились бы совсем другие решения. Но так вышло, что я заинтересовался именно этой прокомментированной сторокой.
Вот уже под конец статьи, задумавшись о том, что раз уж не красиво описывать setter'ы в интерфейсах, то можно представить сборку данных о продукте в виде Builder'а, который после добавления опрелённых данных возвращает другой интерфейс. Опять таки всё зависит от сложности логики построения объектов. Если вы работали с Spring Security, то вам знакомы такого рода решения.
Для моего примера выходит:
public class Application_2 {
public static void main(String[] args) {
var product = new Product.Builder()
.id(1000)
.price(20)
.availability("available")
.images(List.of("url1, url2"))
.build();
System.out.println(product.getId());
System.out.println(product.getAvailability());
System.out.println(product.getPrice());
System.out.println(product.getImages());
}
static class Product {
private final int price;
private final long id;
private final String availability;
private final List<String> images;
private Product(int price, long id, String availability, List<String> images) {
this.price = price;
this.id = id;
this.availability = availability;
this.images = images;
}
public int getPrice() {
return price;
}
public long getId() {
return id;
}
public String getAvailability() {
return availability;
}
public List<String> getImages() {
return images;
}
public static class Builder implements ProductBuilder, ProductWithPriceBuilder {
private int price;
private long id;
private String availability;
private List<String> images;
@Override
public ProductBuilder id(long id) {
this.id = id;
return this;
}
@Override
public ProductWithPriceBuilder price(int price) {
this.price = price;
return this;
}
@Override
public ProductBuilder availability(String availability) {
this.availability = availability;
return this;
}
@Override
public ProductBuilder images(List<String> images) {
this.images = images;
return this;
}
public Product build(){
var av = price > 0 && availability != null ? availability : "sold out";
return new Product(price, id, av, images);
}
}
public interface ProductBuilder {
ProductBuilder id(long id);
ProductBuilder images(List<String> images);
ProductWithPriceBuilder price(int price);
Product build();
}
public interface ProductWithPriceBuilder{
ProductBuilder availability(String availability);
}
}
}Так что:
- Пишите чистые функции
- Пишите красивый и понятный код
- Помните, что краткость — сестра и что главное, чтобы код работал
- Не стесняйтесь ставить вещи под вопрос, искать иные пути и даже забрасывать альтернативные решения, если уж на то пошло
- Не молчите! Во время объяснения проблемы другим, рождаются лучшие решения (Rubber Duck Driven Developent)
