June 14, 2015

Clean and SOLID Java EE code in practice (part 2 of 3)


Pages: 1 2 3

YAGNI: You ain’t gonna need it

Violating YAGNI means implementing functionality which is not actually needed. This is typically KISS applied on a broader scope. Principle violation typically comes from either pre-implementing features which are not yet needed which later turn out to not be needed at all, or from applying outdated or inappropriate design patterns based on vague, improper understanding of their actual application.

The former is prevented by properly applying agile software development practices. 

That latter is also known as the cargo-cult programming anti-pattern. It shows that improper application of best practices, or lack of understanding of best practices can actually turn code to the worse, which may severely damage software maintainability.

Example

As an example, the Java EE stack with its long history offers quite a few possibilities to apply improper or outdated design principles. Some of the most common misconceptions I have come across in my career resolve around application of both the DAO and the DTO design pattern. As Adam Bien states in his blog, both the DAO as well as the DTO have nowadays lost their meaning as the original use cases have been wiped from the Java EE landscape with newer Java EE editions, in short:
Of course, there may be other considerations in your specific case; but generally speaking, implementing DAOs or DTOs must be well justified nowadays.

In the example landscape, one could thus typically cut off both DAO and DTO layers, which shortens an architecture like this:


into this:

Yes, we can get rid of a whole layer as well as of massive information duplication just by using state-of-the-art technology and adhering to pragmatic best practices. Adam Bien identifies a lot more “retired design pattern” when working with a Java EE 6+ stack.

Lessons learnt here is that rather than sticking to whatever someone may have heard being promoted as a “best practice” some time ago, you have to investigate and understand the underlying problem, and you have to do so typically at project start. YAGNI inconveniently introduces a great amount of technical debt into the overall landscape which costs a lot of effort before finally getting killed off.

OCP: Open / closed principle

This principle can be formulated as “making components open for extensions while leaving them closed for changes”, i.e. you should be able to add new functionality without changing existing code.

Although this is a leading principle of basic inheritance mechanisms, I see it violated very frequently.

Example

In its most basic form, this violation is signaled by inappropriate use of the instanceof operator.

For instance, say that we need to determine the maximum of books a customer can borrow simultaneously based on the type of customer. Consider this implementation:
public int calculateMaxNumberOfSimultaneousBorrows(Customer customer) {
    if (customer instanceof TrialCustomer) return 1;
    if (customer instanceof NormalCustomer) return 5;
    if (customer instanceof PlatinumCustomer) return 10;
    throw new IllegalArgumentException("Customer type not supported: " + customer.getClass());
}
I assert that this implementation is easily broken. Whenever a new customer is added, this code needs to be modified. This is exactly what violates OCP: an extension in one place enforces a change in another place, namely in this method. But how is the developer supposed to remember to update this method when he adds a new customer subtype? This implementation could also be considered a violation of SoC (Separation of concerns) as well as a violation of the LSP (Liskov substitution principle). The fact that Java doesn’t allow to switch over classes makes the code even harder to maintain.

This is in fact the classic use case for inheritance, which can here be implemented like so:
public int calculateMaxNumberOfSimultaneousBorrows(Customer customer) {
    return customer.getMaxNumberOfSimultaneousBorrows();
}
where the #getMaxNumberOfSimultaneousBorrows() method is defined in the abstract base class:
public abstract class Customer {
    public abstract int getMaxNumberOfSimultaneousBorrows();
}
and implemented in each concrete subclass, e.g.
public class NormalCustomer extends Customer {
    @Override
    public int getMaxNumberOfSimultaneousBorrows() {
        return 5;
    }
}
Adding a new customer type now is easy as it provides the definition of its #getMaxNumberOfSimultaneousBorrows() method itself, without requiring any changes in existing code.

Actually, this makes the #calculateMaxNumberOfSimultaneousBorrows() (which would be implemented either statically or in a service layer bean) superfluous, because we basically changed the architecture style from service-oriented to object-oriented which in general suits an OO-based language like Java better.

The main lesson learnt here is that except for a few cases of low-level reflection-based programming, usage of instanceof is always a sign (a “code smell”) of bad inheritance / bad OOP application.

Similar OCP violations include switch/case-like conditional choices over magical ints, Strings or enums.

SRP: Single responsibility principle

SRP is about identifying a unit’s (e.g. class’s) responsibility, and implementing functionality which is not part of its responsibility in other parts of the system so that a class should have only one reason to exist, and one reason to change. This helps us achieve high cohesion, which is a measurement of how closely related a single unit’s responsibilities are.

It’s another principle which I see violated very often. This typically leads to lowered cohesion, which results in “god classes” with cluttered functionality, many lines of code and decreased maintainability.

Example

In this example, we want to return an address in a printer-friendly format. Here is the code:
public static class AddressFields {
    private String street;
    private String co;
    private String zipCode;
    private String city;
    private Country country;
    
    @Override
    public String toString() {
        StringBuilder builder = new StringBuilder();
        builder.append(street).append("n");
        appendToBuilderUnlessEmpty(builder, co);
        builder.append(zipCode).append(" ").append(city).append("n");
        appendToBuilderUnlessEmpty(builder, country.getLocalizedName());
        return builder.toString();
    }
    
    private static void appendToBuilderUnlessEmpty(StringBuilder builder, Object object) {
        if (!Strings.isEmpty(object)) {
            builder.append(object);
            builder.append("\n");
        }
    }
}
The good thing is that the code is object-oriented rather than service-oriented: The AddressFields knows how to pretty-print itself through the #toString() method (although I would consider using a less fragile, explicitly user-defined method).

The bad thing is that now, String concatenation logic is cluttered in the address entity which is actually a completely different concern. This class now obviously has two main responsibilities: Holding address information and String formatting, which clearly does not belong here.

In order to become SRP compliant, move String formatting to a dedicated class:
public class StringFormatter {
    public static String join(String separator, String... elements) {
        return Arrays.stream(elements).filter(it -> it != null && !Strings.isEmpty(it))
                .collect(Collectors.joining(separator));
    }
}
(Implementation detail: empty (null) elements are ignored and do not introduce a separator.)

Now, the entity class is free of any String manipulation logic:
public static class AddressFields {
    private String street;
    private String co;
    private String zipCode;
    private String city;
    private Country country;
    
    @Override
    public String toString() {
        return StringFormatter.join("\n",
                street, co, zipCode, city, country.getLocalizedName()
        );
    }
}

SoC: Separation of concerns

SoC is about separating and encapsulating discrete parts of the system into separate, autonomous units, shielding their internals from other parts though information hiding.

When applying separation of concerns, it is key to strive for loose coupling as well, which is a measurement of how closely interweaved autonomous units of a system are. If coupling is high, benefits from separation of concerns are minimized as proper encapsulation is broken.

Example

One of the most prominent examples of separation of concerns actually is the MVC pattern, separating the model, view and controller parts of a system from each other. I’m afraid providing examples of violating this pattern would deserve a blog post of its own, thus I will here only cover one “classic case” of the “model” being mingled with the “controller”.

Given there’s a controller which holds a list model:
public class ReservationController {
    private List<Reservation> reservations;

    public List<Reservation> getReservations() {
        return reservations;
    }
}
How would a service method look like which is in charge of saving all the controller’s reservations to the database? Well, this is the wrong way:
public class ReservationService {
    public void save(ReservationController reservationController) {
        List<Reservation> entities = reservationController.getReservations();
        for (Reservation entity : entities) {
            save(entity);
        }
    }
    
    ...
}
Because #save(…) takes the whole ReservationController as its argument, the two components are tightly coupled, and the service invades into the controller and its internal concerns.

Instead, of course, ReservationController will just pass its model (the reservation list) to the service:
public class ReservationService {
    public void save(List<Reservation> entities) {
        for (Reservation entity : entities) {
            save(entity);
        }
    }
    
    ...
}
Originally, I’ve met this kind of SoC violation on a JSF Facelet page where it was slightly harder to identify. In order not to clutter this article’s with XHTML code, I presented a purely Java-based implementation here. Note, however, that most of these anti-patterns can be applied on any code artifact.

Pages: 1 2 3