Let's Refactor Some of My New, Bad Code: Part 1

I've been on a refactoring tear lately, having refactored some code I found on the Internet for use in a rainflow application and some old code of my own from one of my college courses. I had so much fun doing those posts that I thought I'd round out the trilogy with one more refactoring exercise, this time on some code I've written recently, and horribly. The code comes from the JavaScript code I wrote for this blog in the Everyday DSP for Programmers series. All of the DSP graphs in that series used the HTML5 canvas and the PixiJS 2D rendering library to draw the moving plots that I used as visual aides. I wrote quite a bit of crappy JavaScript, or rather, copied quite a bit of crappy JavaScript to make those graphs. I unapologetically copied my code from one graph to another and added tweaks to get what I wanted for each example animation, making that the most non-DRY code I've ever written.

It's time to clean up that code, and I'm going to walk through those changes here as I make them. My goals for this exercise are a little more focused because I don't expect to have many issues with the formatting or naming this time around. The code is too recent for that, as I tend to be happy with the formatting and naming of my more recent code. I do expect to address the following issues, though:

• There was a problem with activating animations on touch screens that should be fixed.
• There are magic numbers all over the place. Those should be contained in constants and configurations. Shame on me 
• There's duplication all over the place; shame on me again (and again and again).

What I really want to do here is make an API for drawing animated graphs. I also wanted to make tests for this API, since the current code has no tests, but I quickly discovered that writing automated tests for code that draws in a browser canvas is no easy feat. The part of the code that I could easily test without spending weeks of my precious evenings figuring it out was trivial, so I'm going to go with the good old-fashioned method of testing. I'm going to use my eyes. They were build for this kind of visual inspection anyway. I think it's time to get started.

Fixing Touchscreen Support

As it turns out, I already have a small, ugly start to the API because at some point during the writing of the DSP series, I got tired of copying all of that code from one graph to another and seeing it bloat more and more. I started pulling out some of the easiest, most used functions into a separate include file. This file lives in my github.io repo as dsp_graphing.js, and it contains some useful functions for drawing different types of axes and a sine curve generator. It's a start, but it's filled with magic numbers specific to drawing graphs at the size I was using for my blog posts, and, well, yeah, it's really ugly. We'll fix it up, though. The github.io repo will show the refactorings that were done to improve it.

Before we get into the nitty-gritty of refactoring and fleshing out the API, I want to fix that first issue of my graphs not reacting to touch events. I spent a lot of time trying to figure this out the first time writing all of these DSP posts, and I spent a fair amount of time again now, finally figuring it out. Like all good bugs, it was pretty simple in the end. It turns out I was trying to attach an event handler to a canvas event that didn't exist.

How did this happen? Well, PixiJS allows you to add event handlers to different drawing objects in the canvas you're drawing on, and the events have names like 'click' for a mouse click or 'tap' for a touchscreen tap. I was using these names, but I was attaching the handlers to the canvas instead of a PixiJS drawing object because I couldn't get the events to work when I attached the handlers to a drawing object. The 'click' event happened to work because it's the same name for the HTML5 canvas, but the corresponding 'tap' event for the canvas is 'touchend'. Once I figured that out and made the fix, I finally had support for touch screens. Here's what the initialization code looks like for starting up PixiJS on a canvas and adding events:

$(function() {
  var canvas = $('#canvas-sine-offset');
  var renderer = PIXI.autoDetectRenderer(canvas.width(), canvas.height(), { antialias: true });
  canvas.append(renderer.view);
  canvas.on('click', onClick);
  canvas.on('touchend', onClick);
  $('canvas').css('border-radius', 5);
  
  // ...
});

This initialization code is the same for every graph, so I'm going to pull it into a function in my API:

function initCanvas(id, eventHandler) {
  var canvas = $(id);
  var renderer = PIXI.autoDetectRenderer(canvas.width(), canvas.height(), { antialias: true });
  canvas.append(renderer.view);
  canvas.on('click', eventHandler);
  canvas.on('touchend', eventHandler);
  $('canvas').css('border-radius', 5);

  return renderer;
}

Now I can call this function to initialize the canvas for each graph like so:

  var renderer = initCanvas('#canvas-sine-offset', onClick);

There's a problem brewing here that should be addressed. As I continue to add functions to my API, I'm actually adding them to the global namespace. The more functions I add, the more likely I'm going to get collisions with other names in the global namespace. I should wrap up these API functions in a module so they can be accessed through the module name instead. That's easy enough to do:

var dsp_graph = (function() {

  return {
    initCanvas: function(id, eventHandler) {
      var canvas = $(id);
      var renderer = PIXI.autoDetectRenderer(canvas.width(), canvas.height(), { antialias: true });
      canvas.append(renderer.view);
      canvas.on('click', eventHandler);
      canvas.on('touchend', eventHandler);
      $('canvas').css('border-radius', 5);

      return renderer;
    }
  }
}());

I can add the other API functions into this list of functions in the return block, and then I have to go and change all of the calls to these functions on every blog post to include the module name, e.g. dsp_graph.initCanvas. It's a lot of tedious work, but it'll be worth it as I add more functions to the API and refactor the functions that are already there to extract configuration variables and constants. Once that's done, I can inspect the graphs for correctness and commit the code.

Refactoring the Current API

Things are already looking better. Now it's time to tackle the current API monstrosity and make it a little more sane. The main part of the API is a set of four functions that draw different types of axes for the graphs. The first one is drawPositiveAxis():

    drawPositiveAxis: function(stage, x_labels, y_labels) {
      var graphics = new PIXI.Graphics();
      graphics.lineStyle(1, 0xdddddd, 1);
      graphics.moveTo(15, 280);
      graphics.lineTo(535, 280);

      for(var x = 41; x <= 535; x += 26) {
        graphics.moveTo(x, 275);
        graphics.lineTo(x, 280);
      }

      graphics.moveTo(15, 20);
      graphics.lineTo(15, 280);
      for(var y = 20; y < 280; y += 26) {
        graphics.moveTo(15, y);
        graphics.lineTo(20, y);
      }

      stage.addChild(graphics);

      var label_style = { font: '10px Arial', fill: '#eeeeee' };
      var axis_style = { font: 'italic 14px Arial', fill: '#eeeeee' };
      if (typeof x_labels != 'undefined') {
        var x = 55;
        var step = 52;
        x_labels.forEach(function(label) {
          var text = new PIXI.Text(label, label_style)
          text.x = x
          text.y = 282
          stage.addChild(text)
          x += step
        })
      } else {
        var x_text = new PIXI.Text('t', axis_style);
        x_text.x = 540;
        x_text.y = 272;
        stage.addChild(x_text);
      }

      if (typeof y_labels != 'undefined') {
        var y = 223;
        var step = 52;
        y_labels.forEach(function(label) {
          var text = new PIXI.Text(label, label_style)
          text.x = 2
          text.y = y
          stage.addChild(text)
          y -= step
        })
      } else {
        var y_text = new PIXI.Text('f(t)', axis_style);
        y_text.x = 10;
        y_text.y = 2;
        stage.addChild(y_text);
      }
    },

We can start at the top by extracting functions for drawing each individual axis:

  function drawXAxis(graphics, start, end, y) {
      graphics.moveTo(start, y);
      graphics.lineTo(end, y);

      for(var x = start+TICK_STEP; x <= end; x += TICK_STEP) {
        graphics.moveTo(x, y-TICK_SIZE);
        graphics.lineTo(x, y);
      }
      graphics.moveTo(X_AXIS_MID, y - 2*TICK_SIZE);
      graphics.lineTo(X_AXIS_MID, y);
  }

  function drawYAxis(graphics, start, end, x, offset) {
      graphics.moveTo(x, start);
      graphics.lineTo(x, end);

      for(var y = start+offset; y <= end; y += TICK_STEP) {
        graphics.moveTo(x, y);
        graphics.lineTo(x+TICK_SIZE, y);
      }
  }

I was pragmatic here about using constant values instead of parameters where those parameters would not have changed over all of the function calls for the four different types of axes. If I was making a more general purpose API, I would have made things like TICK_SIZE and TICK_STEP parameters, but this is still a specific API for these graphs so I kept the parameter lists simple. I also defined a few more constants at the top of the module for the function call arguments:

  const AXIS_COLOR = 0xdddddd;
  const TICK_SIZE = 5;
  const TICK_STEP = 26;
  const X_AXIS_START = 15;
  const X_AXIS_MID = 275;
  const X_AXIS_END = 535;
  const Y_AXIS_START = 20;
  const Y_AXIS_MID = 150;
  const Y_AXIS_END = 280;

That gets rid of quite a few magic numbers, and the function calls can replace all of the axis drawing code, while using the new constants, like this:

    drawPositiveAxis: function(stage, x_labels, y_labels) {
      var graphics = new PIXI.Graphics();
      graphics.lineStyle(1, AXIS_COLOR, 1);
      drawXAxis(graphics, X_AXIS_START, X_AXIS_END, Y_AXIS_END);
      drawYAxis(graphics, Y_AXIS_START, Y_AXIS_END, X_AXIS_START, 0);
      stage.addChild(graphics);

After replacing all of the axis drawing code with these function calls, what's left is code for adding text titles and labels to the axes. This code can also be extracted into helper functions and named constants:

  const LABEL_STYLE = { font: '10px Arial', fill: '#eeeeee' };
  const AXIS_TITLE_STYLE = { font: 'italic 14px Arial', fill: '#eeeeee' };
  const X_AXIS_TITLE_X = 540;
  const X_AXIS_TITLE_Y_LOW = 272;
  const X_AXIS_TITLE_Y_MID = 142;
  const X_AXIS_TITLE_Y_HIGH = 77;
  const X_AXIS_LABEL_X = 273;
  const X_AXIS_LABEL_Y_MID = 152;
  const X_AXIS_LABEL_Y_HIGH = 87;
  const Y_AXIS_TITLE_X = 10;
  const Y_AXIS_TITLE_Y = 2;

  function drawXTitle(stage, y) {
    var x_text = new PIXI.Text('t', AXIS_TITLE_STYLE);
    x_text.x = X_AXIS_TITLE_X;
    x_text.y = y;
    stage.addChild(x_text);
  }

  function drawXLabel(stage, y) {
    var xLabel = new PIXI.Text('1', { font: '14px Arial', fill: '#eeeeee' });
    xLabel.x = X_AXIS_LABEL_X;
    xLabel.y = y;
    stage.addChild(xLabel);
  }

  function drawXLabels(stage, labels) {
    var step = 2*TICK_STEP;
    var x = step + 3;
    labels.forEach(function(label) {
      var text = new PIXI.Text(label, LABEL_STYLE);
      text.x = x;
      text.y = Y_AXIS_END + 2;
      stage.addChild(text);
      x += step;
    });
  }

  function drawYTitle(stage) {
    var y_text = new PIXI.Text('f(t)', AXIS_TITLE_STYLE);
    y_text.x = Y_AXIS_TITLE_X;
    y_text.y = Y_AXIS_TITLE_Y;
    stage.addChild(y_text);
  }

  function drawYLabels(stage, labels) {
    var step = 2*TICK_STEP;
    var y = Y_AXIS_END - step - 5;
    labels.forEach(function(label) {
      var text = new PIXI.Text(label, LABEL_STYLE);
      text.x = 2;
      text.y = y;
      stage.addChild(text);
      y -= step;
    });
  }

A few magic numbers were left in these functions, but they are contained in functions dedicated to a single task. Adding more named constants like Y_AXIS_END_LABEL_OFFSET seemed like overkill to me. They'll only ever be used in single locations inside these functions, and if they ever need to be adjusted, it's easier to find them where they are used. With these functions extracted, the code for drawing axes has gotten dramatically simpler and easier to understand, e.g.:

    drawPositiveAxis: function(stage, x_labels, y_labels) {
      var graphics = new PIXI.Graphics();
      graphics.lineStyle(1, AXIS_COLOR, 1);
      drawXAxis(graphics, X_AXIS_START, X_AXIS_END, Y_AXIS_END);
      drawYAxis(graphics, Y_AXIS_START, Y_AXIS_END, X_AXIS_START, 0);
      stage.addChild(graphics);

      if (typeof x_labels != 'undefined') {
        drawXLabels(stage, x_labels);
      } else {
        drawXTitle(stage, X_AXIS_TITLE_Y_LOW);
      }

      if (typeof y_labels != 'undefined') {
        drawYLabels(stage, y_labels);
      } else {
        drawYTitle(stage);
      }
    }

The code definitely looks cleaner, and some useful functions have been found that are reused in multiple places. It's arguable that I should have made the parameters for similar functions, like drawXTitle() and drawYTitle(), more parallel by including all similar parameters, but I took the path of simplest function calls instead. This is not a public part of the API, and if there are parameters that would remain constant, I might as well leave them out of the parameter list.

That pretty much wraps up the refactoring of the current DSP graphing API. Between that cleanup and fixing the issue with touchscreen support, we're already making noticeable improvements. After checking that the graphs still look right in my graph inspection pages and committing the code, I'm ready to update all of those blog posts with the new code. Next time we'll start walking through the code in each post and seeing what can be extracted into the API and shared between all of the graphs. I bet we don't get too far before we find some significant improvements.