Don't leave broken windows

“Don’t leave broken windows”. The first time I read this phrase, in the seminal book The Pragmatic Programmer, was probably the most important aha moment of my career. Do you know what it means?

It is (scientifically?) demonstrated that, when a building is abandoned, it can resist a long period of time without people noticing it. But there is a moment when a crucial event happens…that changes everything. This event is a broken window. The broken window won’t be fixed, because the building is not maintained by anybody, so in a short period of time everybody will know about this situation, vandals will start ruining new windows, breaking into the building, and it will be a complete mess very, very soon.

The same idea can be applied to Software development. When we take certain shortcuts to deliver something in the shortest period of time, and our code reflects how careless we’ve been, developers coming after us (from the same team, a future team, and even ourselves!), will reach an important conclusion: it’s not important to pay enough attention to the code we produce. Little by little our application will start deteriorating in what it becomes an unstoppable process.

Levels of broken windows

Not everything is black or white, while there are certain issues that must be addressed immediately, some others can be considered later. Following the metaphor of the building, it’s not the same to have a broken window in the first floor or in the 20th floor of a skyscraper. This is why I think there are two levels of broken windows:

• Stop the world: you have absolutely no excuse to fix this. Not doing it will have serious consequences sooner rather than later.
• Next iteration: assuming we work following an agile methodology, this problem is something that should be addressed as soon as we begin the next iteration / sprint. In order to remember it, we need to add it as an action to the tool we use to track our work (Trello, Pivotal Tracker, JIRA…).

Examples

I’ll review a series of problems that I’ve been seeing over my last few years working as a developer, and which can be classified clearly as broken windows.

README file

Having a README file in our codebase is essential. Sometimes people refers to the Agile Manifesto, which says we should favour "Working Software over comprehensive documentation”, and they translate it as “We don’t need to document”.

This is atrocious. Please, from day one, write and maintain a README file containing basic information such as:

• Brief description of our application (two or three lines, what it’s doing and what problem it solves).
• How to build the project.
• How and when to deploy the project.
• How to test the code automatically (and, if necessary - I hope it’s not - manually).
• What dependencies our system has (database, message broker, etc), and how to connect to them.
• Versioning strategy.
• Conventions to follow (commit messages format, coding style guide, etc).

I’m probably missing some things. The idea is that, if somebody joining our project is not able to ramp up quickly without a lot of support from the rest of the developers (a minimum support will be always required, of course!), it’s because something is missing in our README file.

Too much documentation (and outdated)

It’s the counterpoint to the previous example. In the first stages of the project we may write tons of documentation, containing requirements, design, etc, in a Wiki, Word document, etc, and as the workload increases we don’t maintain those documents, which will be automatically transformed in a source of confusion.

In general, I think that it’s important to have a document containing the high level overview of the architecture of our system, but the implementation details must be in just one place: the code. Every time you start writing a new document that you know will take some time, think about the value you’re adding and the “time debt” you’re acquiring. A “time debt” is nothing but a concrete action that will require additional time from us in the future. You can read more about this here.

Dead Code

This is code that remains in our codebase but it's not invoked by anybody, therefore it's useless. Many people decide to leave that code "just in case I need it in the future". I did this a lot in the past, and the truth is that I never saw myself recovering this code. In addition, there's something called source control :), which facilitates enormously the work to recover code implemented some time ago.

With the current tools it's very simple to detect dead code, and removing it is super simple. So no excuses.

 Commented out code

Being a subset of "dead code", this is even more aberrant. Please, never ever leave commented out code in your software.

 Ignored tests

These are tests that failed one day, and in order to continue doing what we were doing without breaking our CI environment, we added a pretty @Ignore without clearly specifying the reason. In the future, when somebody sees the @Ignore, it can happen that he removes it, and after seeing that the test fails the annotation is added again. So there it remains, forever.

I only see a case where this can be reasonable, and it's when we want to push code "in progress" while we work in a new feature. This code won't be exposed to the final user but it will be integrated in master, and the new feature won't be activated until the test passes. Any other case is just a broken window.

Several dependencies to do the same thing

A developer decides to add a library to do a certain task, let's say working with JSON, and she chooses Jackson.

Later on, another developer who need to work with JSON, decides to add GSON. So we have two libraries to do the same thing!

This transmits a clear message, lack of communication within the team. To avoid this kind of problem, it's very important to carry out Code Reviews. If it's too late, and both libraries are living together in the same project, we should choose one and replace the other for consistency as soon as possible.

Inconsistent code formatting

Who hasn't seen code like this?

public class DishImpl implements Dish {

    private final String name;
    private boolean mixed = false;
    private boolean cooked = false;
    private final List<Ingredient> ingredientList = Lists.newArrayList();

    public DishImpl(String name)
    {
        this.name = name;
    }

    public void add_ingredient(Ingredient ingredient)
    {
      Validate.notNull(ingredient);

      System.out.printf("%s - Adding ingredient %s%n",
                        name,
                        ingredient.getName());
      ingredientList.add(ingredient);
    }

    public void mix()
    {
        if(ingredientList.isEmpty())
        {
            throw new IllegalStateException(
                "There are no ingredients to mix");
        }

        System.out.printf("%s - Mixing ingredients: %s%n",
                            name,
                            ingredientList.toString());

        mixed = true;
    }

    public void cook() {
        if(!mixed)
            throw new IllegalStateException(
                "Ingredients are not mixed, please call mix first");

        System.out.printf("%s - Cooking...%n",
                            name);

        cooked = true;
    }

    public void serve()
    {
      if(!cooked)
        throw new IllegalStateException(
            "Dish is not cooked");

      System.out.printf("%s - Serving...%n", name);

    }
}

(I know, the code is quite stupid)

We see several inconsistencies in this code, like two different ways of using the curly braces (new line in some places, same line in others), two different ways to indent the code, two different approaches to organize if statements with a single instruction (using curly braces vs. not using curly braces), different naming conventions (camel case vs. snake case), etc.

It's extremely important that all developers agree in the style guide to follow, and respect it. My opinion on this is that you should never know who wrote a particular piece of code based on its style.

It's quite easy to find good style guides on the Internet, for example the one from Google.

 Insufficient test coverage

If you practice TDD this shouldn't be a big issue, but in case you don't it can happen that our tests only cover 60-70% of our production code (and even less than that!!).

In the acceptance criteria of our user stories it should be clear that a story won't be delivered if the test coverage decreases. This test coverage should never be below the threshold 90-95%. And if this happens, we should analyze the reasons (too much pressure maybe or just slopiness?).

In any case, the test coverage shouldn't be a goal by itself, but a measure of quality, so please take care with this!

 Manual actions in deployments

The deployment of a new version of our project should be 100% automated. Ideally we should have a task in our CI environment that performs all the necessary actions, and that only requires to push a button from us. Any additional action is just a waste of time that should be analyzed for automation.

 Unstable CI

When our CI environment is in read more time than in green, it's because we're doing something wrong. With this I mean that 95% of the times our tasks (in Jenkins, Bamboo, Concourse, etc) should be executed successfully. If this doesn't happen we have to stop anything we're doing to make it green again.

 Unused imports

This is very typical and easy to avoid. We add an import statement to our class file because we need it at a certain point in time. Later on, we remove the code related to that import (because we refactor the code and we move it somewhere else, for example), leaving the import, which is now completely useless. It's not rare to see a block of 25 imports where only 5 or 6 are actually used.

Removing unused imports is quite simple in the current IDEs. There are shortcuts that can do it instantly, and there's even the option to activate some options that permanently scan our code and remove the unused imports for us.

Unused dependencies

While the previous example can probably be included in the second category ("next iteration"), having unused dependencies in our code that are not used anymore opens the door to potential problems that can be incredibly hard to detect, due to conflicts with transitive dependencies, etc.

As soon as you stop using a dependency, remove it from your project!

 Unclear Git strategy

Without having clear guidelines of how to work with Git, every developer can make his / her own decisions, and we may find ourselves with a Git repository full of branches (many of them dead after being merged or rebased into master), lots of merges, etc.

I'm not going to propose an ideal solution to work with Git here. There are several strategies (aka workflows), but in any case you should leave the decision to every individual in your team.

Dirty Code

We all know what dirty code is, and as professionals we should never deliver features implemented in an unsustainable way, making the maintenance extremely hard due to unreadable code. If you haven't done it yet, please read Clean Code or Code Complete to know exactly what I'm talking about.

This is not Technical Debt

In this post I haven’t talked about Technical Debt, which is a completely different thing. I’m talking about being professionals and taking care of our work! So please don’t forget this mantra every time you sit in front of a screen: “Don’t leave broken windows”.