Broken Window Theory & Boy Scout Rule

In the year 2000, authors Andrew Hunt and David Thomas, in one of the books that are considered an "essential" for any developer nowadays, "The Pragmatic Programmer: From Journeyman to Master", talked about the "Broken Window Theory" applied to software programming. This theory suggests that depreciation of things, like buildings (or software, in our case), suffers from entropy, a physical term that can also be described as the "disorder" in a system and that it seems to be always escalating. So, if you break a window in a building, the tendency is that people will think it's abandoned and will probably wreck it or misuse it. The same can be said for a project in software development: if you keep leaving bad code or practices here and there, people will think that this is acceptable and the snowball keeps growing until the point your whole codebase is infected.

A big example is the lack of tests in parts of the code. Most of us work with tight deadlines, being pressured to deliver as quickly as possible and sometimes it's the last day of your sprint, the customer is waiting for a new feature and you end up shipping it without creating unit tests. To pass the coverage tests your CI runs, you put an ignore flag in that new class and everything's ok.

Let's say, next sprint, a colleague of yours needs to increment the functionality in that class. She develops it, runs some manual tests, everything seems to be working. She creates a pull request and your CI system doesn't complain about tests or coverage. It's strange, but the ignore flag is there, she may think there's a reason for this class to not have unit tests and the snowball gets bigger.

Another example is the inherent copy-paste of code in a big system. Someone does one thing in a bad way, using the wrong structures or abstractions and when other colleague needs to do an activity with a logic alike, he'll just do it in the same way, spreading bad practices to several parts of the code.

To give it an example, let's say we have this code:

public persistItems(): Promise<any> {
  const items = this.getItems();
  for (const item of items) {
    if (!Boolean(item.field1) && !item.field3) {
      return service.createItem(item);
    } else if (Boolean(item.field1) && item.field3) {
      return service.deleteItem(item);
    } else if (item.field7) {
      return service.updateItem(item);
    }
  }
}

This piece of code has a lot of room for improvement. All these conditions nested inside a for loop are not ideal, and they don't really say what is being tested, as they use some database naming (field1, field3, field7). This is a broken window, as people who need to change a condition here or add a new state will probably just add a new if or a new boolean condition, making it even harder to understand and maintain the code.

For improving this, we can use some refactoring techniques. We'll basically use Replace Conditional with Polymorphism to allow persistItems code to be as simple as possible and encapsulate all the conditional logic in a factory that is specifically made for that (Single Responsibility Principle).

So, all of these conditionals will just execute one line of code, responsible for persisting items in one way or another. So we can create an interface for this persistable objects:

export interface Persistable {
  persist(): Promise<void>;
}

Then we can create a class for each of our items, that implements this interface. I'll exemplify just one of them:

export class NewItem implements Persistable {
  constructor(private item) {};

  public persist(): Promise<void> {
    return service.create(this.item);
  }
}

Okay, we have our behavior divided into several classes, but this doesn't resolve our problem yet. We need to create our factory to encapsulate the conditionals:

export class ItemFactory {
  public make(item: Item): Persistable {
    if (this.isNewItem(item)) {
      return new NewItem(item);
    } else if (this.isDeletedItem(item)) {
      return new DeletedItem(item);
    } else if (this.isUpdatedItem(item)) {
      return new UpdatedItem(item);
    }
    return new UnchangedItem(item);
  }

  private isUpdatedItem(item: Item): boolean {
    return item.field7;
  }

  private isDeletedItem(item: Item): boolean {
    return Boolean(item.field1) && item.field3;
  }

  private isNewItem(item: Item): boolean {
    return !Boolean(item.field1) && !item.field3;
  }
}

Here, we are able to extract each condition (isNewItem, isUpdatedItem,...) and keep them readable for everyone. Besides that, we introduce an UnchangedItem, which is a Null Object, so in case our item doesn't need to be persisted, it'll just do nothing with it.

Okay, so after all that, let's see what changed in our method persistItems:

public persistItems(): void {
    const items = this.getItems();
    const itemFactory = new ItemFactory();
    for (const item of items) {
      const persistableItem = itemFactory.make(item);
      persistableItem.persist();
    }
  }

For each item, we'll just call .persist() and we'll let the factory decide which class should be instanced and each behavior should be called. Whenever someone needs to add a new type of item, they have a clear path on where this should be (ItemFactory) and you can keep better track of all the conditions just looking at that class.

There are some ways to deal with the broken window theory. First of all, you should work to prevent it. If there's code that doesn't meet the project standards, for one reason or another, it should be flagged and refactored as soon as possible. It's ok if something needs to be shipped without tests for deadline reasons, but, ideally, the first task of your next sprint should be to unit test that and prevent more broken windows.

Second of all, you should apply the Boy Scout Rule. This one says that you should always leave the code you're working on better than when you got it. This is really important, as, in most scenarios, refactoring and quality improvements are not the priority to a project manager or for the customer. They want the product delivered on time, with working features. Bad code is something that affects mainly us, developers, as it slows down and complicates our own work, so refactor shouldn't be a backlog item or a JIRA task, but an everyday work of tidying up our code as we work through it. So whenever you need to add something and you find a broken window, fix the code first, don't try to put your feature inside a mess of a code, as it'll only tangle it up more and more.