The worst code I ever saw was when I was called in to finish the work of a consultant who had left the company for greener pastures. The feature had been in development for 4 months, and was, according to the consultant who left, about 1 week away from being finished.

However, I had some trouble wrapping my head around his Java code.

• There were design patterns everywhere. For instance, the ExtendedDataSearchStrategyFacade was apparently intended to implement the DTO pattern, the strategy pattern, and the facade pattern, all at once (which doesn't make any sense, since they solve totally different design problems)

• Delegation was rampant. When the user clicked the search button, this request was passed through 6 different classes from 3 maven modules before being delegated to the existing service layer.

• Dependency injection was everywhere. For instance, DTOs were consistently instantiated by declaring a dependency on a spring prototype bean defined by a Spring ObjectFactory that delegated to a method in a DTOFactory interface he had written. This interface was implemented by an abstract class AbstractDTOFactory that restated every method with an abstract modifier, which was in turn subclassed by the DTOFactoryImpl which delegated to the static factory method in the DTO, which delegated to the private constructor, which did nothing.

• Every public method to receive a parameter of reference type began by checking that this parameter was not null, and did nothing otherwise:

    public void initFoo(Bar bar) {
       if (bar != null) {
           this.foo = // some complicated code that uses bar
       }
    }
  

  Needless to say, this allowed null values to propagate stealthily throughout the entire program. For instance, foo was null, because initFoo did nothing, because bar was null, because initBar did nothing, because baz was null, because initBaz did nothing, because service was null, because it was commented out in the dependency injection config.

• All String literals in the entire project were centralized in a class StringConstants, used, for instance, like this:

    String displayName = StringConstants.LEFT_PARENTHESIS 
                       + person.getLastName() + StringConstants.COMMA 
                       + StringConstants.SPACE + person.getFirstName()
                       + StringConstants.RIGHT_PARENTHESIS;
  

  I have never been as glad for the "inline constant" refactoring as I was that day, which turned this into:

    String displayName = "(" + person.getLastName() + ", " + person.getFirstName() + ")";
  

• Hidden beneath all that accidental complexity were the real logic bugs. For instance, to search all entities where x and y were in some range, he did this:

    for (int x = minX; x < maxX; x++) {
       for (int y = minY; y < maxY; y++) {
          list.add(dtoFactory.newExtendedSearchRangeDto(x,y));
       }
    }
    // in another method, after passing list through several classes:
    for (ExtendedSearchRangeDto d : list) {
       service.findEntityByXAndY(d.getX(), d.getY());
    }
  

  Depending on user input, this would have issued up to 1 million SQL queries in sequence (which might explain why the search was "slow" the one time it was demoed to management).

The lesson I learned from this code was that more design patterns don't necessarily make a design better, but can also make it worse. Design patterns are standard solutions for standard problems, but if you don't have the problem, solving it is a waste of effort - and takes focus away from the things that actually matter (such as that million SQL queries ...)

Management, by the way, was astounded to learn that his code was poorly designed, for he used to speak at length about how much he cared for design and his extensive pattern knowledge ...

posted about 4 years ago

CC BY-SA 4.0

meriton‭

2375 reputation 7 56 220 35