June 14, 2015

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




Writing software to deliver functionality is not enough in the real world. Software has to be maintainable right after its initial programming, and typically for many years after its release. Luckily, there are some well-established design patterns and best practices which help us achieve high software quality. Rather than publishing another article about how things are done in the perfect world, I will here present some real-world examples of violated or misinterpreted design patterns I found throughout my career – and how to fix them.

In this article, I will cover nine of the most well-known (object-oriented) software design principles, five of which make up the famous S.O.L.I.D. principles. They are:
I see some of these principles violated quite often in everyday programming. For each principle, I will here present a particular violation or misconception, based on what I have observed in a software development project I was involved, and show how the design flaw can be fixed by applying true best practices.

To achieve some uniformity, I came up with a fictional example project the following code samples will be embedded in: It’s an online portal of a library where clients can make book reservations.

KISS: Keep it simple, stupid

“Keep it simple, stupid” is probably the most important software design principle. It shortens meetings and acts as an advocate for clean, readable code.

KISS typically increases ease of initial programming, but it may decrease maintainability if overly or improperly used.

In general, I’ve found that there is a serious misconception about this principle, turning it into “Keep it stupid” which is not what the principle is about. In this case, it is used as an excuse not to elaborate a cleaner abstraction and hence introduces code which typically violates DRY and other clean code principles.

Example

As an example, I will try to demonstrate the constraints of KISS applicability.

For instance, given this simple requirement: When the user queries for a library book he’d like to borrow, a simple availability check is performed which, if failed, leads to an error message. Here’s how you could implement the logic to return an error message:
public String checkBoookAvailableSimple(String name) {
    Book book = service.getByName(name).get(0);
    if (book == null || book.getCurrentReservation().isActive()) {
        return "book.reservation.check.notAvailable";
    }
    return null;
}
This is KISS in action. We use a very simple syntax and basic JDK classes; there is no further need to encapsulate or abstract things.

Now imagine a the requirement becoming more complex: When the user queries for a library book he’d like to borrow, multiple validation checks are performed. Some of them are considered critical, immediately yielding an error message whilst others just add warning messages which are collected and once all validation checks are completed, are returned in a list.

Rigorously following the KISS principle, one could continue to implement the logic like so:
public List<String> checkBoookAvailable(String name) {
    List<String> messages = new ArrayList<>();
    List<Book> books = service.getByName(name);
    if (books.isEmpty()) {
        messages.add("book.reservation.check.noMatch");
    }
    else if (books.size() > 1) {
        messages.add("book.reservation.check.duplicate");
    }
    else {
        if (books.get(0).getCurrentReservation().isActive()) {
            messages.add("book.reservation.check.currentReservation.active");
        }
        if (books.get(0).isDamaged()) {
            messages.add("book.reservation.check.damaged");
        }
    }
    ...
    
    return messages;
}
However, this code has tremendously increased in complexity. There are complex conditional branches, and there is mutable state (as held by the messages list).

(As we will see later, this code also violates SRP (Single responsibility principle) by mixing book reservation logic with validation message building.)

This code is hard to extend and maintain. This really is a naïve approach for the problem. It is KIS (“keep it stupid”) because the developer is in charge of managing condition flow and state, which makes it error-prone.

By introducing some level of abstraction, the code can by enormously simplified:
public List<String> checkBoookAvailable(String name) {
    List<Book> books = service.getByName(name);
    BookReservationValidator validator = new BookReservationValidator();
    return validator
            .check(books.isEmpty(), "book.reservation.check.noMatch")
            .ifOkCheck(books.size() > 1, "book.reservation.check.duplicate")
            .ifOkCheck(books.get(0).getCurrentReservation().isActive(), "book.reservation.check.currentReservation.active")
            .check(books.get(0).isDamaged(), "book.reservation.check.damaged")
            ...
            .getMessages();
}
Here, I introduced the BookReservationValidator class which manages error messages and provides a simple API, based on the builder pattern, to run new checks and query the messages.

This code is far easier to maintain thanks to the self-documenting nature of the BookReservationValidator API.

(And it respects SRP. Validation message building is now encapsulated in BookReservationValidator and can be extended and tested independently of the concrete “book reservation” business case.)

Of course, this code example still is kept very simple for illustrational purposes. It’s really borderline over-engineering here. As a general rule, the more complex functionality is, the more dangerous it is to drift from KISS to KIS. A fluent interface, e.g. based on the builder pattern, may significantly increase maintainability in this case.

DRY: Don’t repeat yourself

Simply put, DRY is the opponent of “copy-paste programming” which is in general considered a very poor and potentially very dangerous coding practice as it seriously hampers maintainability.

I would judge this as my personal second leading principle right after KISS. Actually, DRY is what you should strive after, but in certain situations, eradicating any code duplication just isn’t worth the effort, would make a solution too complicated or is not even possible. Common examples are XML or JSF Facelets pages.

DRY typically demands more initial effort, but increases overall maintainability. Given these properties, it really is a counterforce of the KISS principle. On stackoverflow, I found the wonderful quote to always “keep your KISSes DRY”. In good programming practice, these two principles should not compete, but complement each other:


While I see DRY typically respected when implementing common static methods or the like, I’ve observed people struggling to apply it on a higher abstraction level in particular.

Example

Say for instance, we have client contact information which is threefold, consisting of contact, invoice and delivery information, which could be rendered in the GUI like so:



A very naïve approach to map this into a business model class would be to model each single information as an individual field of the business model, like so:
public class Information {
    private String contactStreet;
    private String contactCity;
    private String contactZipCode;
    private String contactPhone;
    private String contactEmail;
    
    private String invoiceStreet;
    ...
}
In UML terms, this would be rendered like so:

A contact information consists of 11 fields. When ORM-mapped into a DB table, this object would be represented as a single table row.

There is no inner structure, which makes the code hard to read, and there is potentially a lot of code duplication because instead of defining once what a street, a city or a state is, and then referring to it three times, we have to define it three times individually. This designs favors copy-paste-coding which is error-prone and hard to maintain.

Imagine, for example, that there is the functionality to auto-fill city when the zip code input loses focus. With this naïve approach, as there is no abstraction nor generalization, one has to implement this functionality for each individual field, like so:
public class ContactInformationController {
    public void updateContactCity(ContactInformation model) {
        String city = zipCodeService.getCity(model.getContactZipCode());
        if (city != null) {
            model.setContactCity(city);
        }
    }
    
    public void updateInvoiceCity(ContactInformation model) {
        String city = zipCodeService.getCity(model.getInvoiceZipCode());
        if (city != null) {
            model.setContactCity(city);
        }
    }
    ...
}
Did you find the copy-paste error which crept in? Anyways, this is very bad programming style. Programming really is about building abstractions, where appropriate, and not doing so leads to bad code quality.

Worst of all, this implementation does not match the reality of the business model. If we closely observe the business model as it is defined, it is apparent that we actually do have reuse of components, and we should make sure that the code reflects the business model. Thus we can identify these patterns:


Note that:
  • The blue parts are re-occurring components
  • The lighter blue parts are optional extensions to a single blue component
With Java’s basic OO mechanisms, we can easily model these relationships:
  • Composition for re-occurring parts
  • Inheritance for extensions
What I have in mind is a class diagram which looks like this:

Which is then implemented like this:
public class ContactInformation {
    private ContactFields contact;
    private AddressFields invoice;
    private AddressFields delivery;

    public static class AddressFields {
        private String street;
        private String city;
        private String zipCode;
        ...
    }
    
    private static class ContactFields extends AddressFields {
        private String phone;
        private String email;
        ...
    }
}
Of course, in the view, the input box for the street of the contact address would now e.g. map to the nested property of contactInformation.contact.street.

Because we now have the address encapsulated in a separate entity, we can implement the “update city” functionality by working with the abstraction, thus getting rid of code-copy-pasting:
public class ContactInformationController {
    ...
    public void updateCity(AddressFields model) {
        String city = zipCodeService.getCity(model.getZipCode());
        if (city != null) {
            model.setCity(city);
        }
    }
}
This design has an overwhelming number of advantages:
  • The structure of the business model source code matches the structure of the real world equivalent, including the structure of the UI, making the code easier to read.
  • The “container” objects are very simple. The main container, ContactInformation, really is just a POJO which doesn’t care about the sub-containers it consists of. This aligns with the SoC (Separation of concerns) principle (see below) and makes the design easily extensible.
  • Code duplications are minimized. Even in the GUI, you can probably (depending on the technology) build a component template for an AddressField (blue part) and include it three times with different parameters.
  • As for persistence concerns: because the individual parts are now modeled to independent entities, you are free to lower transaction boundaries to the level of individual address containers and to use lazy loading. Because you’ll typically use an automated ORM framework, the complexity introduced on the database level shouldn’t be an issue.
Before you go and abstract everything, however, still keep KISS in mind.

Pages: 1 2 3