Best practices for refactoring a functioning JavaScript web app?

[chrisofarabia]

About 2 years back, I started on one of General Assembly’s ‘Web Development Immersive’ courses. During that, we were required to complete a number of projects, the first of which had to be a game. I’d like to use that project as the base for understanding more about the best way of tackling the refactoring of a functioning web app.

For various reasons, I’ve needed so step back from coding as a career choice, and have inevitably become somewhat rusty. I would though like to get my head back into that space again, and both re-learn and add to what I think I once knew.

The application is a re-creation of ‘Minesweeper’, the Github for all the code being found here - https://github.com/christopherallanperry/WDI_PROJECT_1

And a working installation located here - https://frozen-eyrie-56010.herokuapp.com

The kind of things I’d like to understand are:

1. Can the overall structure be improved?
2. Everything is declared at the global level, so how best to move away from that.
3. Could more use be made of ES6?
4. Might modules be appropriate in such a setting?
5. Perhaps not exactly critical, but I’ve always suspected the same result could be achieved in rather less lines of code - is that realistic?

I’m not particularly looking for code, more the approach and better ways to think about things, but if examples would help that, then I’m happy to see what comes up.

I’d like to think that this might become a useful resource for other members in the future, or at least something that could be distilled down into that.

Is anyone game to give it a go?

[Paul_Wilkins]

@chrisofarabia

Is anyone game to give it a go?

That’s something I could have a look at. I should have some time next week to investigate.

[chrisofarabia]

I’d be most interested in your thoughts.

[motor_man]

Unit and integration testing
Rebuild using test driven development (TDD)
Use ES6
Rebuild implementing a modern JavaScript framework such as; Angular, React, or Vue
Add server-side api to track actions, build reports and insights on activity. Also build this with TDD.
Host in cloud and build automated CI pipeline for deployment.

[m3g4p0p]

Hey @chrisofarabia, one point for improvement would be that you’re deriving the application state from previous DOM modifications at a couple of places; for example:

if (this.getAttribute('class') === 'tile flag'){
  // ...
}

if (document.querySelectorAll('[data-checked]').length === 0) {
  // ...
}

This is rather inefficient and makes the code difficult to reason about. Better would be to keep the state in the application itself; the view merely reflects that state and triggers state updates via events. For example, you might introduce a Tile component that encapsulates the state of being checked, flagged or mined, and updates the DOM where necessary… along the following lines (just as a sketch for the general idea):

class Tile {
  constructor (value) {
    this.value = value
    this.isChecked = false
    this.isMined = false
    this.isFlagged = false    
    this.element = document.createElement('li')
    this.element.classList.add('tile')
  }

  mount (target) {
    target.appendChild(this.element)
    this.element.addEventListener('click', () => this.checkTile())
    this.element.addEventListener('contextmenu', () => this.toggleFlag())
  }

  placeMine () {
    this.isMined = true
  }

  checkTile () {
    this.isChecked = true
    this.element.style.backgroundColor = '#eee'
    // ...
  }

  toggleFlag () {
    this.isFlagged = !this.isFlagged
    this.element.classList.toggle('flag', this.isFlagged)
    // ...
  }
}

Similarly, you might move some logic to a Grid component that holds the tiles and is responsible for things like calculating the number of touching mines, and an App component at the top level that holds the grid and the game controls; these components would then all be separate modules (so yes that’s definitely appropriate). :-) Communication between parent and child components can be achieved with callbacks, like e.g.

class Tile {
  constructor ({ value, onToggleFlag, onCheckTile }) {
    this.value = value
    this.onToggleFlag = onToggleFlag
    this.onCheckTile = onCheckTile
    // ...
  }

  checkTile () {
    // ...
    if (this.onCheckTile) {
      this.onCheckTile(this)
    }
  }

  toggleFlag () {
    // ...
    if (this.onToggleFlag) {
      this.onToggleFlag(this)
    }
  }
}

Alternatively you might introduce a central dispatcher to which components can publish and subscribe, although this is probably not necessary for such a relatively shallow component tree.

[chrisofarabia]

Did I say I was rusty?

I do get the basic idea though. Think I’ll need to sit with it a while and get my head around how to introduce such ideas into the game, as it looks like it could require some fairly radical surgery, rather than the outpatient care I’d imagined.

[mawburn]

Honestly?

Take the functionality and rewrite it. That’s really the only way I’ve seen it done or done it. Nothing really magical about it. That’s what I’m doing now, that’s what I’ve done before, and that’s what I’m sure I’ll do 50 more times before I retire.

Generally old code is too hard, been modified too many times, and too convoluted to really leverage much from. If you’re lucky, the person who made it made some libraries that were heavily abstracted and agnostic that you could leverage for core things, but even that is rarely the case.

If you’re talking more tech specific, I really like React/Redux with Typescript. I didn’t really look at your code, but based on @m3g4p0p’s reply is that it’s more HTML centric than JS where as React is more JS centric. Everything exists in your JS and you’re not storing anything in the DOM other than the things you want rendered.

[chrisofarabia]

In this case it’s all my own code and hasn’t been touched since I wrote it, so no patches, or tinkering, or libraries for that matter; just plain old ‘vanilla’ JS rendering stuff out onto the DOM as the player interacts with the game matrix. From that PoV it’s very simple, but could almost certainly be improved, even if it does work perfectly well as is.

In a sense, the exercise is less about the refactoring per se, but getting my head into better ways of thinking about code organisation. The last year or so has put that kind of stuff onto the back burner, but I’d now like to see if I can get myself back into it in a meaningful way.

[Paul_Wilkins]

@chrisofarabia

In this case it’s all my own code and hasn’t been touched since I wrote it, so no patches, or tinkering, or libraries for that matter; just plain old ‘vanilla’ JS rendering stuff out onto the DOM as the player interacts with the game matrix. From that PoV it’s very simple, but could almost certainly be improved, even if it does work perfectly well as is.

In a sense, the exercise is less about the refactoring per se, but getting my head into better ways of thinking about code organisation. The last year or so has put that kind of stuff onto the back burner, but I’d now like to see if I can get myself back into it in a meaningful way.

I find that one of the beneficial ways are to attune yourself to what are called code smells.

Elijah Manor has done a good but fast 30-minute talk about it, but the slides from his page are for the 60-minute talk so you can get some good details from them there.

JavaScript Code Smells