Notifications
Q&A

What is the worst code you ever saw? [closed]

+1
−12

closed as too generic by ‭Mithical‭ on Sep 26, 2020 at 19:43

This post contains multiple questions or has many possible indistinguishable correct answers or requires extraordinary long answers.

This question was closed; new answers can no longer be added. Users with the reopen privilege may vote to reopen this question if it has been improved or closed incorrectly.

In the interest of learning from the mistakes of other people:

What is the worst code you ever saw? What made it so bad?

Why should this post be closed?

4 comments

Might one of the down or close voters be so kind as to give more specific feedback why they disliked this question? The close reason given is a bit ... generic ... ‭meriton‭ about 1 month ago

I'm not one of the down/close voters - but to me the questions seems like it doesn't have any problem to solve and wants to collate anecdotal answers, which would be better suited for a forum discussion than a Q&A site. That being said, I did learn something from your answer. ‭jla‭ about 1 month ago

I closed this post because it's asking for personal anecdotes; there's an infinite number of possible answers to such a question, and as such doesn't fit so well into a Q&A format. It might be better suited to chat, or possibly a series of blog posts (which you might want to discuss in the Meta category). See also How to ask a great question in the Help Center. ‭Mithical‭ about 1 month ago

1 answer

+3
−2

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 ...

1 comment

Design patterns are largely a solution looking for a problem. The original GoF book was worthwhile, in part, but the susbsequent attempt to reduce the whole of computer science to design patterns was not. ‭EJP‭ 15 days ago