The program code is written from top to bottom. Instructions are read and executed in the same order. This is logical and everyone has long been used to it. In some cases, you can change the order of operations. But sometimes the sequence of function calls is important, although syntactically this is not obvious. It turns out that the code looks working, even after rearranging the calls, and the result is unexpected.
Once, a similar code caught my eye.
Problem
Once, poking around in someone else’s code in a joint project, I discovered a function like:
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; }
Of course, it’s not the most “elegant” style that immediately catches your eye: classes store data (POJO), functions change incoming objects ...
In general, it seems to be nothing. We have an object in which there is not enough data, and there is data itself that came from other sources (services), which we will now place in this object to make it complete.
But there are some nuances.
- I don’t like the fact that not a single function is written in the style of FP and modifies the object passed as an argument.
- But let's say that this was done to reduce the processing time and the number of created objects.
- Only a comment in the code says that the sequence of calls is important, and you should be careful when embedding the new
Filler
- But the number of people working on the project is more than 1 and not everyone knows about this trick. Especially new people on the team (not necessarily in the enterprise).
The last point especially guarded me. After all, the API is built so that we do not know what has changed in the object after calling this function. For example, we have a Product::getImages
method before and after, which, before calling the fill
function, will produce an empty list, and then a list with pictures to our product.
With Filler
things are even worse. AvailabilityFiller
does not make it clear that it expects that the price information of the goods is already embedded in the transferred object.
And so I thought about how I could protect my colleagues from the erroneous use of functions.
Proposed solutions
First, I decided to discuss this case with my colleagues. Unfortunately, all the solutions they proposed did not seem to me to be the right approach.
Runtimeexception
One of the proposed options was: and you write in the AvailabilityFiller
at the beginning of the function Objects.requireNonNull(product.getPrices)
and then any programmer will already receive an error during local tests.
- but the price really may not be there, if the service was unavailable or some other mistake, then the product should receive the status of "out of stock". We’ll have to attribute all sorts of flags or other things to distinguish “no data” from “not even requested”.
- If you throw an exception in
getPrices
itself, then we will create the same problems as modern Java with lists
- Suppose a List is passed to a function that offers the get method in its API ... I know that you don’t need to change the transferred objects, but create new ones. But the bottom line is that the API offers us such a method, but in runtime an error may occur if it is an immutable list, such as that obtained from Collectors.toList ()
- If you throw an exception in
- if
AvailabilityFiller
is used by someone else, the programmer who wrote the call will not immediately understand what the problem is. Only after launch and Debug. Then he still has to understand the code to figure out where to get the data.
Test
"And you write a test that will break if you change the order of calls." those. if all Filler
return a “new” product, something like this will turn out:
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));
- I do not like tests that are so "White-Box"
- Breaks with every new
Filler
- It breaks when changing the sequence of independent calls
- Again, it does not solve the problem of reusing
AvailabilityFiller
itself
Own attempts to solve the problem
Idea
I think you already guessed that I would like to solve the problem at the compilation level. Well, why do I need, one wonders, a compiled language with strong typing if I cannot prevent the error.
And I wondered if an object without additional data and an "extended" object belong to the same class?
Wouldn't it be right to describe the various possible states of an object as separate classes or interfaces?
So my idea was this:
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)
Those. the product originally defined is an instance of a class other than the returned one, which already shows data changes.
Filler
, in their APIs, specify exactly what data they need and what they return.
This way you can prevent the wrong sequence of calls.
Implementation
How to translate this into reality in Java? (Recall that inheriting from multiple classes is not possible in Java.)
Complexity is added by independent operations. For example, pictures can be added before and after adding prices, as well as at the very end of the function.
Then maybe
class ProductWithImages extends BaseProduct implements ImageAware{} class ProductWithImagesAndPrices extends BaseProduct implements ImageAware, PriceAware{} class Product extends BaseProduct implements ImageAware, PriceAware, AvailabilityAware{}
How to describe all this?
Create adapters?
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; }
Copy data / links?
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; }
As already noticeable, it all comes down to just a huge amount of code. And this despite the fact that in the example I left only 3 types of input data. In the real world, there can be many more.
It turns out that the costs of writing and maintaining such code do not justify themselves, although the idea of ​​dividing the state into separate classes seemed very attractive to me.
Retreat
If you look at other languages, then somewhere this problem is easier to solve, but somewhere not.
For example, in Go you can write a reference to an extensible class without "copying" or "overloading" methods. But it's not about Go
Another digression
While writing this article, another possible solution came up withProxy
, which called for writing only new methods, but requiring a hierarchy of interfaces. In general, scary, angry and not suitable. If someone is suddenly interested:
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()); } } }
Output
What is it? On the one hand, there is an interesting approach that applies separate classes to an "object" in different states and guarantees the prevention of errors caused by an incorrect sequence of calls to methods that modify this object.
On the other hand, this approach makes you write so much code that you immediately want to refuse it. A heap of interfaces and classes only makes it difficult to understand the project.
In my other project, I still tried to use this approach. And at first at the interface level, everything was just fine. I wrote the functions:
<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){}
Having thus indicated that a certain data processing step requires additional information that is not needed either at the beginning of the processing or at its end.
Using mock
'and, I managed to test the code. But when it came to implementation and the amount of data and various sources began to grow, I quickly gave up and remade everything into a “normal” look. Everything works and no one complains. It turns out that the efficiency and simplicity of the code triumphs over the “prevention” of errors, and it is possible to trace the correct sequence of calls manually, even if the error manifests itself only at the stage of manual testing.
Maybe if I took a step back and looked at the code from the other side, I would have completely different solutions. But it so happened that I became interested in this particular commented line.
Already at the end of the article, thinking about the fact that since it’s not nice to describe setters in interfaces, you can imagine the assembly of product data in the form of Builder
, which returns a different interface after adding the defined data. Again, it all depends on the complexity of the logic of constructing objects. If you worked with Spring Security, then you are familiar with this kind of solution.
For my example, it goes:
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); } } }
So that:
- Write clean features
- Write beautiful and clear code
- Remember that brevity is a sister and that the main thing is that the code works
- Feel free to question things, look for other ways and even throw up alternative solutions, for that matter
- Do not be silent! When explaining a problem to others, better solutions are born (Rubber Duck Driven Developent)