Readable code – why it really matters

It’s a known fact that programmers spend more time reading code than actually writing it. A good, precise, clean and readable code may actually save us a lot of time, aid in reducing annoying bugs, ease with the understanding of the flow and allow easier modifications. The keyword is easy. Unless you are assembly programmer or writing some life critical code, readable or good structured code should be your number one priority. A good number of books such as “Code Complete”, “Clean Code / Architecture”, “Refactoring” are dedicated to that topic.

I work now in a small-medium sized startup, CompanyX, with ~20 folks that has learned that the hard way. Around 2 years before my first teammate was hired, our company had hired a team of freelancers from foreign country. Some of those guys still work with us and I must admit they are pretty good as developers, although they like to complicate stuff. The code might be way too generic and abstract without any reason, a usage of design patterns that seem to complicate more that they solve, etc. As my previous boss once said:

There are very smart guys that will construct you a plane even though they have been asked to build a simple stool

• some smart guy

My teammates actually developed a conspiracy theory that these guys just try to maximize their job security. Maybe you too after you see some code snippets.

The consequences

Although we are considered to be a startup we have come to a phase where we have had a lot of dependencies between our features, It means that almost any new feature is based upon much older features, so in order to understand how to write new stuff, we need to understand the mechanisms behind the old ones. In other words, the proportion between the code we read and the code we write is constantly increased. Therefore, old code has a serious impact on the team and the company overall. So here is a list of the effects I’ve notices:

• Extremely slow delivery and low productivity Tasks that take days in my current position, took hours in my previous startup! Tech debts were ignored because we had to concentrate on delivery to the client. Classical Catch-22).

• Tech depts are not embraced happily I see tech debts as tasks or features that ease developer’s life and probably have a potential save a lot of time and money in the future. Tech debts also allow us to prepare the ground for future features. In CompanyX, tech debts are considered almost a taboo because we are told that they don’t provide any value to the client.

• The word “Refactoring” is heard too much Perhaps tech debts are not welcomed, but “Refactoring” is the most pronounced word you can hear in our office. And it has a good reason for that. The sad thing is that almost nobody knows the actual meaning of it.

• Low quality code triggers additional low quality code My teammates have a habit to blame everything on the remote team of freelancers. What they don’t realise is that some of them are writing a poor code themselves. Personally, I’m in a constant struggle with our poor architecture, because each design proposal for a new feature seems to me like a hack. For example: I was told to add new features dealing with rare race conditions, performance improvement etc. My conclusion is that most of them could be avoided by using a better architecture.

• Developers don’t understand the business and basic flows 1 year of experience in ‘CompanyX’ is just not enough. The code is over complicated that it takes hours to understand simple flows. Today, I consult only with my CTO, PM or the foreign guys because I don’t trust my teammates to know stuff.

• Developers fear of making big and necessary changes One of them even said that the way of working is by having minimum changes. It’s sad that it was said by a developer with a 10 years of experience. In my opinion, such methodology will not make you a better developer and can even harm you.

• Low quality code that resulted in a contract cancelation with a client Now bugs may appear from time to time, but if the code is poorly written and documented, then you are going to spend a lot of time by just figuring where to start. Since our app works on premise, the client was supposed to install it locally. Well, he just couldn’t do it. In a desperate attempt, we sent our CTO and product so they could somehow track the issue, but sadly nothing worked. We lost half of our clients in an instant because we had only one and burned a lot of money by sending people abroad. If money is the air for startups, then its reputation is the water. And we lost both. I believe we cannot afford additional errors in the future.

• Low morale and motivation among the developers and the team People just don’t care. They mock the app without shaming. Moreover, it became part of our unofficial company’s lexicon: “fast as our ‘CompanyX’!”, “There are no bugs in ‘CompanyX’ because it’s a bug of its own”, etc.

• Bye bye fullstack We were promised to write feature in both client and server side, but due to slow delivery, each one was allowed to take only one and his strongest niche. That’s a deal breaker for some of us.

• Intense relationship between R&D and PO / PM You just feel it in the air.

Robert C. Martin in his “Clean Architecture” has already described some of these bullets in his first 2 chapters. Authors such as Martin Fowler popularised the practice of refactoring model

So who said books are useless nowadays?

Ugly code top 5

We are programmers and not philosophers, so let’s see some code examples. The rating is reversed in its order.

5. Dozens of inner classes

Honestly, I’m not a big fan of nested classes in Java, however sometimes they seem to make sense in case you want to implement a data structure or if you have a code that is used only by the its parent class. Anyway, using dozens of inner classes by making the whole file to be hundreds of lines is a little bit annoying and misses the basic concept of separating the codebase into different files. Sometimes you can find even references from outer classes to the nested ones of a different class, which is the wrong thing to do:

public AggregatorService {
  public void doSomething() {
    ActionImpl.InnerAction action = this.makeAction(this.lengthValue);
    Boolean result = action.apply();
  }
}

There are multiple examples of classes which are flooded with dozens of smaller nested classes. I believe we can argue about that because it may sound as a question of style, but think of it: why do we have packages in Java, modules in Javascript, crates in Rust etc?

You can even find more absurd examples like an interface with it’s only implementation as its inner class:

public Interface ActionProcessor {
  void consumeAction();
  boolean message();

  class DefaultActionProcessor implements ActionProcessor {
    private Action action;
    private Properties properties;

    @Override
    public void consumeAction() { /* do something */ }
    public boolean message() { /* do something */ }
  }
}

Now, writing interfaces and adding abstractions is all cool. But in case there is only one implementation which is used only inside the package, what abstraction do you need to achieve? What is the purpose of this nested class usage? To spare a file? These questions were asked by many folks in my company.

Ok, we’ve just finished our warmup 🙂

4. And more nested code…

Haven’t I mentioned we have an Angular client side too? I guess the nested virus also infected our front-end:

I got some super nested file which contains multiple components, services, pipes and even a module with routing! Imagine you are working on a task and need to have some changes in the UI. After debugging your browser, you have finally found the component which needs to be modified. You open your text editor and then you are in a middle of a terror attack:

@Component({
    template:
        `<div>...</div>`,
    styleUrls: ['./some-css-style.scss']
})
//Just a template

export class Setup implements OnInit {
//some init stuff here
}

@Component({
    template:
        `<div>...</div>`,
    styleUrls: ['./some-css-style2.scss']
})
//Just a template

export class Completer implements OnDestroy {
//some cleaning stuff here
}

@Injectable()
export class PredicateResolver implements Resolve<boolean> {
    //Some logic
}

@Injectable()
export class ActionResolver implements Resolve<Action> {
    //Some logic
}

@Injectable()
export class DataResolver implements Resolve<Data> {
    //Some logic
}

@Component({
    template: `
        <div>
           <!-- some template stuff here -->
        </div>
    `,
    styleUrls: ['./at-least-I-am-not-embedded.scss']
})

export class ComponentClass implements OnChanges {
    //component code
}

//additional components, exported classes, interfaces, routes and services

@NgModule({
    imports: [
        //list of imports including routing
    ],
    declarations: [
        //list of declarations
    ],
    exports: [
        //list of exports
    ],
    providers: [
        //interceptors, services etc.
    ]
})
export class SomeModule {
}

3. The visitor pattern:

Lets get back to Java:

public <V extends Visitor> V visit(V action) {
  for (Event event : Events) {
    if (event instanceof StartedEvent) {
      action.onExecutionStartedEvent((StartedEvent) event);
    } else if (event instanceof MessageEvent) {
      action.onMessageEvent((MessageEvent) event);
    } else if (event instanceof ErrorEvent) {
      action.onErrorEvent((ErrorEvent) event);
    } else if (event instanceof TimeoutEvent) {
      action.onTimeoutEvent((TimeoutEvent) event);
    } else if (event instanceof CompletedEvent) {
      action.onCompletedEvent((CompletedEvent) event);
    } else if (event instanceof TerminationEvent) {
      action.onTerminationEvent((TerminationEvent) event);
    } else if (event instanceof TriggeredEvent) {
      action.onTriggeredEvent((TriggeredEvent) event);
    } else if (event instanceof ExceptionEvent) {
      action.onExceptionEvent((ExceptionEvent) event);
    } else if (event instanceof CommonExceptionEvent) {
      action.onCommonExceptionEvent((CommonExceptionEvent) event);
    } 
    // and many more. SOS!
  }
  return action;
}

What was your first thought when you saw this? What does your gut tells you? In two words, what we just see is an event ledger processor. Each visitor has its own implementation for each type of event. Now, the tricky part is that there are dozen of these events (meaning 12 inner “else ifs”), and a little bit less visitors. I’m not considering myself as a stupid person, but it took me a lot of time to understand its mechanism. Additionally, this code is also very hard to debug: I remember how my CTO and I have debugged endlessly in trying to fix a bug in that area of code.

This is how I was feeling while debugging the aforementioned code

2. Stairways to heaven (or hell)

public boolean process() { //A ~200 lines method
  //Some code above
  FlowIterator<Context, Step, Action, Error> visitor =
    new CalculateVisitor<>(
      new FilterVisitor<>(predicate,
        new UnexpectedErrorVisitor<>(
          new SomeActionVisitor<>(datum,
            new SomeActionVisitor<>(
              new SomeOtherAction<>(datum,
                new AggregationVisitor<>(aggregatedParameter,
                  new CalculateOtherStuffVisitor<>(datum, calculatorFunction,
                    new FinalCalculationVisitor<>(calculationArgument,
                      new BuildVisitor(datum)
                    )))))))));
  //more code below
}

“Stairways to heaven” was the name my coworker gave it while my proposal was different: “Chaos is a ladder“. Perhaps the first name is better because you can listen to that wonderful song while debugging the code or because you can guess the irony behind it, because we are actually dealing with a classic example of callback hell. By the way, nested classes strike again: each visitor class (e.g. CalculateVisitor) is actually an inner class!

1. Java class path as a value in database

I consider this one to be the champion of our anti-patterns! One of the most coupled codes I’ve ever seen.

Let’s start with a little review of the architecture, so we can better understand the tragedy behind:

Figure 1 – a sketch of the flow concerning the class path delegation starting from DB A till Service C

As part of a flow, Service A fetched the class path, let’s assume “CP”, from DB A, then it delegates it to Service B, which then delegates it to Service C. Now, Service C takes the string value of the class path, and loads it using Java reflection API.

Figure 2 – a sample of DB A table containing class path values and its constructor arguments of Service C. There are 3 such tables.

And here is the code in Service C:

@Component
public class ClassGenerator {

  @Autowired
  private ApplicationContext applicationContext;

  public Action create(String className, Map<String, String> arguments) {
    try {
      Class<?> clazz = Class.forName(className);
      Constructor<?> constructor = clazz.getConstructor(Map.class);
      Action action = (Action) constructor.newInstance(arguments);
      applicationContext.getAutowireCapableBeanFactory().autowireBean(action);
      return action;
    } catch (Exception e) {
      throw new RuntimeException("What am I doing here?", className), e);
    }
  }
}

Are you aware that a simple class renaming can cause a crash of the entire app?

Not only we are getting a class path of a class from literally nowhere, but also loading it dynamically to Sring’s context… wow!

Oh come on, why couldn’t we just use a simple and well known factory method?

Conclusion

We have seen the consequences of writing unreadable code and how they affect the whole company. We have also saw some coding examples and I hope that you are thrilled as I do.

Even though there are so much negative sides to this situation, I still think I can learn a lot from it. I believe that in order to become a better software engineer, we need to learn not just the good parts, but the bad ones too. Why? Because we improve our ability of distinguishing between these two. After working for almost all of my carrier for companies with a very high quality coding standards, I’ve finally seen in my own eyes their practical importance without any philosophical nonsense. At the end of the day, bad experience is still an experience. However, I don’t think that staying here too long is going to make me a better developer or contribute to my goal of becoming a software architect.