Программный код пишется сверху вниз. В том же порядке читаются и выполняются инструкции. Это логично и все давно к этому привыкли. В некоторых случаях можно поменять порядок операций. Но иногда очерёдность вызовов функций важна, хотя синтаксически это неочевидно. Получается, что код выглядит рабочим, даже после перестановки вызовов, а результат оказывается неожиданным.


Однажды подобный код попался мне на глаза.


Проблема


Однажды, ковыряясь в чужом коде в совместном проекте, а обнаружил функцию наподобие:


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), функции изменяют входящие объекты...


В целом вроде бы ничего. У нас есть объект, в котором не хватает данных, и есть сами данные, пришедшие из других источников (сервисов), которые мы теперь в этот объект и поместим, чтобы сделать его полноценным.


Но есть и некоторые нюансы.


  1. Мне не нравится, что ни одна функция не написана в стиле ФП и изменяет объект, переданный как аргумент.
    • Но допустим, что это было сделано, чтобы сократить время обработки и количество создаваемых объектов.
  2. Только комментарий в коде говорит о том, что последовательность вызовов важна, и при встраивании нового 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, то вам знакомы такого рода решения.


Для моего примера выходит:


Решение на основе Builder-Pattern
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)

Спасибо за внимание