Three levels of refactoring I use daily in Java
Programs, no matter how complex, are just a bunch of if else statements, and the number of if else statements that can model a program is probably the best metric of program complexity. However, if elses are quite ugly. Most of us hate them because they drain our mental capacity when we walk through them, trying to understand what is happening in our code. Therefore, we invented a bunch of programming languages and paradigms just to get rid of them, to hide them beneath abstractions. This is the one of the core aims of OOP as well as FP: to hide if else statements as elegantly as possible.
Yet, the shortest way to implement a new requirement is most often than not to add another if. Let’s tackle the problem through an example:
Assume we have this simple class, that contains some particular details for a product in various languages, pertaining to the countries in which it’s sold:
public class InternationalProduct {
ProductDetails uk = null;
ProductDetails nl = null;
ProductDetails de = null;
ProductDetails fr = null;
ProductDetails es = null;
ProductDetails it = null;
ProductDetails at = null;
ProductDetails ro = null;
record ProductDetails(String name, String description) {}
}A common requirement would be to get the product details name for a given language, for which the naive implementation is inevitably:
Translation getProductName(InternationalProduct product, String language){
if(language.equals("at")){
return new Translation(product.at.name(), new Locale("at"));
}
if(language.equals("nl")){
return new Translation(product.nl.name(), new Locale("nl"));
}
if(language.equals("de")){
return new Translation(product.de.name(), new Locale("de"));
}
if(language.equals("fr")){
return new Translation(product.fr.name(), new Locale("fr"));
}
if(language.equals("es")){
return new Translation(product.es.name(), new Locale("es"));
}
if(language.equals("it")){
return new Translation(product.it.name(), new Locale("it"));
}
if(language.equals("ro")){
return new Translation(product.ro.name(), new Locale("ro"));
}
throw new IllegalArgumentException("Language not supported");
}Where Translation and Locale can be these simple records:
public record Translation(String value, Locale locale) {}
public record Locale(String language) {}Is the above code ugly? I would argue it isn’t. It has the advantage of being easy to read and understood by anyone. As long as there is no processing being done inside “if” statements. Even if a new requirement comes in to extract the description of the product this time:
Translation getProductDescription(InternationalProduct product, String language){
if(language.equals("at")){
return new Translation(product.at.description(), new Locale("at"));
}
// ....
}You can easily make use of the Separation of Concerns principle and just extract the specific product details first, using if/else blocks just in that method and construct the Translation object afterwards. So it has some potential for refactoring, while extending the functionality, let’s say, by supporting a new language, requires the same amount of work as with any desing. Some code needs to change, and adding another if is not the end of the world (with the exception of using config files and java Reflection to access ProductDetails fields, which is in general a terrible idea for multiple reasons like not using static type features of the language, hard to optimize by compiler, slow and error prone).
The thing is, in reality, business logic always gets more complex, and temptation to add one more line of code, the quick fix, way too big, and this approach somewhat encourages things to get out of hand in time. Consequently, I always try to create a design that makes it harder to turn the code into a mess.
One attempt in this direction is to get rid of the is else statements entirely: to use data structures provided by the JDK, namely the Map where the key is the “if”, and the value is the “what”, or the “then” if you’d like. I prefer this option over the first one, as it offers a better separation between the configuration, in our case language mapping, and the code performing the business logic:
public class SlightlyImprovedMapper {
private final Map<String, Function<InternationalProduct, String>> nameExtractors;
public SlightlyImprovedMapper() {
this.nameExtractors = new HashMap<>();
nameExtractors.put("uk", p -> p.uk.name());
nameExtractors.put("at", p -> p.at.name());
nameExtractors.put("nl", p -> p.nl.name());
nameExtractors.put("de", p -> p.de.name());
nameExtractors.put("fr", p -> p.fr.name());
nameExtractors.put("es", p -> p.es.name());
nameExtractors.put("it", p -> p.it.name());
nameExtractors.put("ro", p -> p.ro.name());
}
Translation getProductName(InternationalProduct product, String language) {
return new Translation(nameExtractors.get(language).apply(product), new Locale(language));
}For some, this version is not as easy to read (look at the declaration of the “nameExtractors” variable, but I would argue it is a bit more flexible, especially in the long run, using functional programming. Now implemeting the “getProductDetails” is thought provoking. Clearly, our solution is not flexible enough, and we don’t want to create a new map — that would mean code duplication, a bad practice we struggle to avoid. In general, when we reach this point, we have to name things: the essence of OOP, otherwise things get out of control. What we would want is a map with two values per key (also known as a Touple, each value being a Function<InternationalProduct, String> that can “extract” what we want from our product. As you know, Java doesn’t support Touples by default so we will have to define our own class. With this step, we can also add the language field to our new class, so that we use a List instead of a Map, keeping things as simple as they can be.
You might find it difficult to name such data structures until it is clear in your mind what you use them for, but in the end naming things is the most difficult job we have as programmers (besides cache invalidation), so let’s call the entity “ProductExtractor”:
public record ProductExtractor(String language,
Function<InternationalProduct, String> nameExtractor,
Function<InternationalProduct, String> descriptionExtractor) {
}The nicest thing about OOP is that after the hard part of naming things, everything is clear. The abstract function no longer scares us, because we know what it is, what it should do: extracting name or description. With that in mind, our mapping class can look like this:
private final List<ProductExtractor> extractors;
public ImprovedMapper() {
this.extractors = new ArrayList<>();
extractors.add(new ProductExtractor("uk", p -> p.uk.name(), p -> p.uk.description()));
extractors.add(new ProductExtractor("at", p -> p.at.name(), p -> p.at.description()));
extractors.add(new ProductExtractor("nl", p -> p.nl.name(), p -> p.nl.description()));
extractors.add(new ProductExtractor("de", p -> p.de.name(), p -> p.de.description()));
extractors.add(new ProductExtractor("fr", p -> p.fr.name(), p -> p.fr.description()));
extractors.add(new ProductExtractor("es", p -> p.es.name(), p -> p.es.description()));
extractors.add(new ProductExtractor("it", p -> p.it.name(), p -> p.it.description()));
extractors.add(new ProductExtractor("ro", p -> p.ro.name(), p -> p.ro.description()));
}
public Translation getProductName(InternationalProduct product, String language) {
String name = extractorByLanguage(language)
.map(e -> e.nameExtractor().apply(product))
.orElseThrow(() -> new IllegalArgumentException("Language not supported"));
return new Translation(name, new Locale(language));
}
public Translation getProductDescription(InternationalProduct product, String language) {
String description = extractorByLanguage(language)
.map(e -> e.descriptionExtractor().apply(product))
.orElseThrow(() -> new IllegalArgumentException("Language not supported"));
return new Translation(description, new Locale(language));
}
private Optional<ProductExtractor> extractorByLanguage(String language) {
return extractors
.stream()
.filter(e -> e.language().equals(language))
.findFirst();
}Notice we don’t have any if else statements in the above code. They are there, hidden inside the stream filter, in using Optional functions, but we don’t manage them. In the long run, this makes our code more manageable, as adding a new language with this design does not affect at all the business logic, which is what we wanted in the first place.
If you are a big fan of OOP, and your lambda functions are growing due to changing requirements, there’s one more improvement you might be thinking of and it’s of course, naming things again. You can turn ProductExtractor into an interface and define specific extractors for each language, like UkProductExtractor below:
public final class UkProductExtractor implements ProductExtractor {
private final String language;
private final Function<InternationalProduct, String> nameExtractor;
private final Function<InternationalProduct, String> descriptionExtractor;
public UkProductExtractor() {
this.language = "uk";
this.nameExtractor = p -> p.uk.name();
this.descriptionExtractor = p -> p.uk.description();
}
....
public class ImprovedMapper {
private final List<ProductExtractor> extractors;
public ImprovedMapper() {
this.extractors = new ArrayList<>();
extractors.add(new UkProductExtractor());
extractors.add(new AtProductExtractor());
//...
}If you are using Spring framework and define extractors as beans, you already have a list of ProductExtractor to inject in your services, made available by the magic of Spring. For our simple scenario this is a bit overkill, but in practice I find this technique more often than not suitable to provide improved readability to our code base.
As a conclusion, the best recipe for a quality code is using functional programming to implement business logic, strictly with pure functions in a transformer style approach, then name / encapsulate everything using OOP style programming. I’m curious to know your thoughts on the subject.
