Broken Windows: How to Create Clean Code

Introduction

This time it won't be my favorite rant about certain operating system. Instead, just a few thoughts about the psychology of bad code.

The broken windows theory, an academic theory proposed by James Q. Wilson and George Kelling in 1982, is a metaphor for disorder within neighborhoods. Their theory links disorder and incivility within a community to subsequent occurrences of serious crime. - Encyclopedia Britannica 

The Tale of Shrubbery Code

Once, in the past, me and my team had a discussion about event handlers in front-end code. We looked in bewilderment at some ancient code. Something similar to this Vanilla JS (I'm a big fan of Vanilla JS, to be clear):

    this.btnSave.addEventListener('click', () => {
      window.fetch('/selection', {
        method: 'POST', headers: { 'Content-Type': 'application/json' },
        body: JSON.stringify(this.items.map(item => { id: item.id, selected: item.selected })))
      .then(response => response.json())
      .then({ data, success, error } => {
        if (success) {
          Notification.info('Data saved succesfully');
          this.items = data.items;
        } else throw new Error(error);
      })
      .catch(error => {
        Notification.error(error.message);
        this.close();
      });
    });

This is a fine specimen of shrubbery code. One looks at it and has no clue how to approach it without hurting oneself. Maybe I should read it backwards? How about unit tests? Don't look at these either — it's equally bad code which proves nothing expect that code coverage is at 70% so everybody's happy.

We've discussed the following problem. A programmer was asked to add a Delete button. That's easy! Copy-paste, and... there are two shrubberies now. Then three. Then it's a mess. The only thing one can do at this point is rewrite. So what should I put into an event handler?

I said a single line is enough. There was serious disagreement.

The Rewrite

Here's an even more troubling question — why would a developer plant another shrubbery at all? Couldn't they see how bad it is already? This made me think. What if the initial code looked like this:

    // Submits HTTP request
    submitRequest (endpoint, data, method = 'POST') {
      const body = data ? JSON.stringify(data) : undefined;
      const headers = { 'Content-Type': 'application/json' },
      const options = { method, headers, body };
      return fetch(endpoint, options)
        .then(response => response.json());
    }

    // Handles received response
    handleResponse (operation, { data: { items }, success, error }) {
      if (success) {
        Notification.info(`${operation} succeeded`);
        return items;
      } else throw new Error(`{$operation} failed`);
    }

    // Handles request error
    handleError (error) {
      Notification.error(error.message);
      this.close();
    }

    // Returns current selection
    getSelection () {
      return this.items.map({ id, selected } => ({ id, selected }));
    }

    // Submits SAVE SELECTION request
    submitSelection () {
      const endpoint = '/selection';
      const data = this.getSelection();
      return this.submitRequest(endpoint, data)
        .then(response => this.handleResponse('Save', response));
    }

    // Updates current selection in UI
    refresh (items = []) {
      this.items = items;
    }

    // Handles save button click
    saveButtonClicked () {
      this.submitSelection()
        .then(items => this.refresh(items))
        .catch(error => this.handleError(error));
    }

    // Wires up events
    this.saveButton.addEventListener('click', () => this.saveButtonClicked());

It's far from ideal. There are 10 other ways to structure it. Request handling code sure will be moved to a separate service. But it's a good start. It reads easy. The event handler now just has one line of code, where it all begins. From here I can drill down into the details. Unit tests for these little functions are easy to write.  Another handler for the delete button would be trivial, as it would reuse most of the code.

It gets even better! If I read it backwards, it still makes perfect sense!

Yes, it is more lines of code. But since we're not paid per line-count anymore, this should no longer be an issue for the management. Meanwhile, I can read it and learn in small bits, without an instant brain seizure. I want to write this kind of code!

That was the answer. If the developer in question saw the above, they'd follow the pattern. Unless they wouldn't, but then we have a team problem.

So Why Isn't All the Code Neat Like That?

We all have our coding guidelines, code reviews, books of standards, John Papa, and Todd Motto. Yet we keep churning out shrubbery code. Why?

Guidelines and standards are nice. But we always need to apply them. We should ignore remarks like these and do our job properly:

"Look, this little component has just two event handlers, they're a few lines each and will not be reused elsewhere, so why not keep it concise?"

First, we often mistake concise for compressed. They're not the same. Check https://www.ioccc.org/ for proof.

Finally, the broken windows theory. It's not just about me. It's about everybody else who will someday work on this code, including a future me. How?

Make it a miserable place and whoever comes after me will keep adding to the misery. If I encounter a shrubbery... well, why would I be a hero and get rid of it? Just fix what needs a fix and get the hell out of there. And think twice before cleaning it up. Such heroism usually backfires, as a team instantly assumes that from now on I'm the one responsible for this whole mess.

Put in place a disciplined structure, separate the concerns, apply rules consistently, have JSDoc comments, and proper unit tests — and they will follow. Because it feels wrong to spoil such an idyllic landscape!

For inspiration, here's a great Daniel Stori comic!