June 14, 2015

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


Pages: 1 2 3

LSP: Liskov substitution principle

Barbara Liskov’s principle states that you have to be able to substitute an object by a derived object without breaking the original functionality. Violating the principle damages the very basics of class inheritance which makes the code vulnerable for modification. As such, the LSP can be perceived as a very object-oriented subtype of the OCP.

Example

Here is a rather “classic” violation of the LSP: A subtype is derived from its parent without needing its entire interface. Imagine that we have a book entity that we happily use all over our library application:
public class Book {
    private String name;
    private String author;
    private int pages;
    ...
}
But then one day, we decide, we want to have audio books, too. So the audio book would make a subtype of the book in order to easily extend the application:
public class AudioBook extends Book {
    private int length;
    
    @Deprecated
    @Override
    public int getPages() {
        throw new UnsupportedOperationException("pages property is not supported by audio book.");
    }
    @Deprecated
    @Override
    public void setPages(int pages) {
        throw new UnsupportedOperationException("pages property is not supported by audio book.");
    }
    ...
}
In the class diagram, it looks like this:


But obviously, there’s a problem. For an audio book, the pages information doesn’t make sense. In the book entity, the #getPages() method is expected to return the number of pages, but this cannot be applied to the audio book entity (here, this is also a violation of the ISP (interface segregation principle)).

A “quick and dirty” solution is presented above where the “surplus” methods are marked with @Deprecated and throw runtime exceptions. However, this very implementation would break existing code, as in
public int getTotalPages(List<Book> books) {
    return books.stream().mapToInt(Book::getPages).sum();
}
The #getPages() method should thus rather return 0, but that doesn’t make it anymore right. In a way, the LSP is an “inconvenient” principle because it forces you to reconsider the design of your class hierarchies. In this example case, there are two conceivable solutions with would accord with the LSP as well as with the ISP:

The straightforward one is to create a common super class which only contains the shared properties, making the common interface as small as possible, such as
public abstract class Work {
    private String name;
    private String author;
    ...
}
In the class diagram, it looks like this:

Book (adds pages) and AudioBook (adds length) would then both extend it. Now, the aforementioned #getTotalPages(List<Book>) method would run through LSP-compliant: as it only operates on non-audio books, querying the total of pages would always make sense. However, this approach will probably force you to exchange the Book interface to the more general Work interface everywhere both a book or an audio book are acceptable. On the other hand, this is a very lean solution and is definitely the way to go if you think that one day, you may extend the subtypes of Works even further, as you’re then ready to do so.

Another solution the LSP offers is to use the composition over inheritance design where you turn a (dysfunctional) is-a-relationship in a both-have-a-relationship. In the example use case, you might say that okay, both a book and an audio book have a kind of content; for the book, it’s pages, and for the audio book, it’s audio tracks. Thus you can model:
public class Book {
    private String name;
    private String author;
    private final Content content;
    ...
}
a book having content. And Content being an abstract class which can either be
public class PaperContent extends Content {
    private int pages;
    ...
}
or
public class AudioContent {
    private int length;
    ...
}
In turn, you then have to modify the #getTotalPages(List<Book>) method:
public int getTotalPages(List<Book> books) {
    return books.stream().filter(it -> it.getContent() instanceof PaperContent)
            .mapToInt(it -> ((PaperContent)it.getContent()).getPages())
            .sum();
}
In the class diagram, it looks like this:


Generally speaking, with this approach, you will be able to leave your interfaces untouched, but the changes to the entity’s properties will break the code everywhere they are referenced.

Using composition over inheritance also has some more advantages:
  • You’re able to switch the composite relationship at runtime. If this is not desired, as in the example use case (how would you turn a book into an MP3 file?), you will typically mark the composite reference as final.
  • You’re not restricted to 1-to-n relationships, as you can model any n-to-n-relationship.
Depending on your use case, these advantages may drive you to go for a composition-based approach.

ISP: Interface segregation principle

Allow me to cite Robert C. Martin / Wikipedia directly here: The ISP states that no client should be forced to depend on methods it does not use. This is pretty simple and straightforward, and it makes sense. Bloating an interface with methods which are not used decreases readability, but most importantly, it further increases coupling between client and interface. One should thus always strive after narrow, concise interfaces.

Example

I’ve observed violations of this principle coming from a common misconception of the program to interfaces, not implementations design principle. In Java in particular, there’s the misconception of “interface” referring to a Java interface rather than the conceptual idea of a contract. However, Java EE 6+ in particular promotes usage of the “no interface view” which allows to refer to (and inject) any Java Bean without the technical need to declare an interface for that bean.

The Java EE container doesn’t need an interface to refer to a single bean implementation. Even worse, if there were multiple implementations of this interface, without further specification (e.g. though @Qualifier) the bean container would throw an exception at startup because it finds multiple matches for the interface!

This is an application of cargo-cult programming by itself, leading to the YAGNI violation of creating interfaces for (in the worst case) every public method of a class.

As an anti-pattern example, take this service implementation:
public class ReservationServiceImpl implements IReservationService {
    @Override
    public List<Reservation> getReservations(Client client) {
        …
    }
    
    @Override
    public List<Reservation> getActiveReservations(Date inbetween) {
        …
    }
    
    @Override
    public void addReservation(Client client, Reservation reservation) {
        …
    }
    
    @Override
    public void updateReservation(Reservation reservation) {
        …
    }
    
    @Override
    public void cancelReservation(Reservation reservation) {
        …
    }
    …
}
which implements the IReservationService even though there is no business value nor any technical need which would justify the existence of this interface. This anti-pattern is even worse in a service-oriented architecture where the number of “service methods” is typically very high, as opposed to a pure CRUD service implementation.

To justify the existence of the interface, it would then have to be referred to in a call from any lower layered module, e.g. a “view controller”. This approach does not only make the UML diagram look unnecessarily complicated:


but even worse, that “indirection” from the lower layer to the service layer is real and in everyday programming, it means that you have to navigate from the controller to the service by querying for the one interface implementation; and on every change on the interface implementation, the interface has to be updated. It’s just silly: the interface then depends on the implementation which reduces its existence to absurdity.

Here, the solution is straightforward: get rid of the interface. If the service bean’s implicit interface defines its own implementation anyway, let it take this role. This will reduce the UML to


(You will also probably want to remove the unnecessary “…Impl” suffix.)

DIP: Dependency inversion principle

The DIP is sometimes also referred to as the “Hollywood principle” (“don't call us, we'll call you”) applied to software architecture. It states that low level modules should depend on high level modules’s abstractions rather than the high level modules depending on low level module implementations.

This concept is key to realize loose coupling which in turn strengthens the SoC principle, as discussed earlier.

It is realized through the Inversion of control (IoC) design pattern which is most prominently implemented in a Java EE bean container through dependency injection mechanisms where dependencies on high level modules are “injected” into low level modules through abstractions (implicit or explicit interfaces).

Even though this concept is quite omnipresent in Java EE projects thanks to the “enforced” usage of IoC containers, I see developers struggling to apply the concept outside of the container, thus missing opportunities to loosen coupling between individual components.

Example

As an example, let’s consider the scenario where a user interface consists of a multi-tab view which is backed by a “controller” bean. Now, every time the user changes to a new tab, the content of this new tab should be refetched from the database so as to guarantee that the new view reflects the database’s current status (e.g. for list views: refetch all the entities).

A very naïve approach would be to just update every single view (i.e. its backing “controller”) on tab change:
public class TabController {
    private BookSearchController bookSearchController;
    private ReservationController reservationController;
    private ClientAccountController clientAccountController;
    
    public void onTabChange(TabChangeEvent event) {
        bookSearchController.update();
        reservationController.update();
        clientAccountController.update();
    }
}
Note that here, the actual violation of the DIP is the fact that TabController holds a reference to every single concrete controller which backs one of the view’s tabs. Therefore, dependency is maximized:




Note that there’s not even any need for the various …Controllers to implement a common interface (here, it’s just coincidence that their update methods all share the same name) as we do not depend on a common interface, but on the concrete implementation of each single controller. This is what violates DIP.

Also, what if you want to add a new controller? You need to modify that code; therefore, it also violates OCP.

As a more runime-efficient solution, you may want to update only the go-to-controller by querying TabChangeEvent for a clue as to which controller should be updated. However, as long as the actual controller instance is held by the TabController bean, nothing changes from a DIP / OCP point of view:
public class TabController {
    private BookSearchController bookSearchController;
    private ReservationController reservationController;
    private ClientAccountController clientAccountController;
    
    public void onTabChange(TabChangeEvent event) {
        switch (event.getControllerName()) {
            case "book": bookSearchController.update(); break;
            case "reservation": reservationController.update(); break;
            case "client": clientAccountController.update(); break;
        }
    }
}
(Also, switch on String is terribly bad anti-OCP.)

In order to get rid of the interdependency chaos, you need to find a way for the TabChangeEvent to directly return a reference to the controller which it should update.

Given that you can provide a TabChangeEvent similar to this:
public abstract class TabChangeEvent {
    public abstract BaseController getController();
}
We can then work with a BaseController abstraction which every tab content controller must implement:
public abstract class BaseController {
    public abstract void update();
}
Therefore, TabController doesn’t have to hold any dependency to a concrete tab content controller anymore:
public class TabController {
    public void onTabChange(TabChangeEvent event) {
        event.getController().update();
    }
}
Dependencies are thus minimized:



In a Java EE environment, where controllers are supposed to be container-managed beans, TabChangeEvent would typically return the respective controller instance by bean manager lookup.

Conclusion

Writing software is easy and everyone can do it. Writing maintainable software, however, is hard, and takes years of experience. This is what makes you a professional engineer and a true software craftsman.

Luckily, great minds of the industry have come up with best practices and common design patterns to increase software quality and maintainability. However, I am surprised to find that even in professional environments, knowledge and acceptance of these common best practices is not always present. I am unsure whether this is caused by lack of interest, lack of knowledge or mismanagement.

With this blogpost, I wanted to provide a comparison of anti-patterns and best practices for some of the most well-known software design principles which is backed by practical best practices application.

Please tell me in the comments whether you can relate to this post, whether you found it helpful or if, on the other hand, it lacks important information or contains any errors. I highly appreciate your feedback.

You may also be interested in


Pages: 1 2 3

2 comments:

  1. IMHO the DAO example for YAGNI principle is not correct. That kind of abstraction is still useful. Although I really prefer the repository pattern:
    http://martinfowler.com/eaaCatalog/repository.html

    You linked the Adam Bien post but if you read the comments, some people told him that he is wrong:
    http://www.adam-bien.com/roller/abien/entry/jpa_ejb3_killed_the_dao

    Regards,
    Juan

    ReplyDelete
    Replies
    1. Juan, thank you for your comment. I do actually agree with you. To overcome shortcoming of the EntityManager API, it may make sense to write a (thin) wrapper around it. You may or may not call this wrapper a “DAO”. When I call on “killing the DAO” I really mean not to thoughtlessly build a 1 service class : 1 DAO class architecture just to fulfill some outdated J2EE pattern or to needlessly abstract from a relational database – I have seen just that in a real world project which inspired this section of the article. Maybe I shouldn’t have used the quite “extreme” wording Adam Bien likes to use…

      Regards, Nicolas

      Delete