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

Welcome back to my multi-part post on refactoring some bad code I wrote last year to make it a little less bad and a little more readable. The code is a bunch of copy-and-paste code I wrote for the Everyday DSP for Programmers series on how to do some basic and some not-so-basic signal processing, shown with nifty animated graphs using JavaScript and the HTML5 canvas. The refactoring so far has involved fleshing out the API I had started when I first wrote the DSP series, fixing a couple bugs that I had never gotten around to fixing before, and working my way through the code for the first post in the series, making improvements and cleaning things up. The goal for this post is to pick up the pace because there were 15 parts to the DSP series, and I'm not writing posts for this refactoring series one-for-one with that series.

To speed things up, I'm going to apply refactorings that we've already covered without much explanation, and if some graphs don't show any unique points that I want to make, I'll skip over them. When I first wrote the DSP series, I was trying to focus on the explanations for all of the signal processing techniques, and I got lazy about writing clean code for the graphs. This led to way more copy-and-pasting than there should have been, so a lot of the refactorings will be terribly repetitive. I don't want to show them all, and I'm sure you don't want to read about them all, so we'll stick to the highlights. With that explanation out of the way, let's pick up where we left off with the signal transforms post.

Annotations and Animation Frames

After applying previous refactorings, the code for the first graph, showing how changing an offset changes a signal, looks like this:

$(function() {
  var renderer = dsp_graph.initCanvas('#canvas-sine-offset', onClick);
  var stage = new PIXI.Container();

  dsp_graph.drawZeroAxis(stage);

  var t = 0;
  var amplitude = 2;
  var offset = 0;
  var freq = 1;
  var phase = 0;

  var baseSine = dsp_graph.createCurve(stage);
  dsp_graph.drawSine(baseSine, 0xeeeeee, amplitude, offset, freq, phase);

  var sinewave = dsp_graph.createCurve(stage);

  var baseText = new PIXI.Text('f(t) = sin(t)', { font: 'italic 14px Arial', fill: '#eeeeee' });
  baseText.x = 100;
  baseText.y = 125;
  stage.addChild(baseText);

  var sineText = new PIXI.Text('f(t) = sin(t) + ' + offset, { font: 'italic 14px Arial', fill: '#00bbdd' });
  sineText.x = 100;
  sineText.y = 20;
  stage.addChild(sineText);

  function onClick() {
    if (offset == 0) offset = 2;
    else if (offset == 2) offset = -2;
    else offset = 0;
    sineText.text = 'f(t) = sin(t) + ' + offset;
  }

  (function animate() {
    if (t > offset + 0.01) t -= 0.04;
    else if (t < offset - 0.01) t += 0.04;

    sinewave.clear();
    dsp_graph.drawSine(sinewave, 0x00bbdd, amplitude, t, freq, phase);

    renderer.render(stage);
    requestAnimationFrame( animate );
  }());
});

This code looks mostly okay now. It starts off by initializing the graph and drawing the sine curves with annotations, setting up the click handler that changes the offset values, and defining the function that animates the graph when the offset changes. Even though it's relatively clean, I can see two ways to improve things. First, we're doing a lot of annotation in this post, so we can create another function in the API called annotate() that creates text annotations at a specified location on the graph, like so:

    annotate: function(stage, text, color, origin) {
      var annotation = new PIXI.Text(text, { font: 'italic 14px Arial', fill: color });
      annotation.x = origin.x;
      annotation.y = origin.y;
      stage.addChild(annotation);
      return annotation;
    }

Then the two sections of code that create the PIXI.Text() objects can be rewritten as:

  var baseText = dsp_graph.annotate(stage, 'f(t) = sin(t)', '#eeeeee', {x:100, y:125});
  var sineText = dsp_graph.annotate(stage, 'f(t) = sin(t) + ' + offset, '#00bbdd', {x:100, y:20});

The second issue is not readily apparent in the code, but when you're looking at the animated graphs in this post on a laptop, you may start to notice after a while that your fan starts to spin up. The processor is working pretty hard even after the animation finishes between clicks. Why is that? If you look in the animate() function, you can see that the last line requests another animation frame every single time the function executes, which causes the function to execute again, requesting another animation frame. To make this cycle stop, we need to only request a new frame if we need to update the graph. What determines if we need to update the graph? Well, if t changes, then the graph is going to change, so we can just assign a flag variable to only request a new frame if t changed:

  function animate() {
    var update = true;
    if (t > offset + 0.01) t -= 0.04;
    else if (t < offset - 0.01) t += 0.04;
    else update = false;

    sinewave.clear();
    dsp_graph.drawSine(sinewave, 0x00bbdd, amplitude, t, freq, phase);

    renderer.render(stage);
    if (update) requestAnimationFrame( animate );
  }

  animate();

This change makes it so that the animation is not constantly requesting frames when it's not moving, but it also requires that we no longer use the immediate invocation syntax for the animate() function. Instead, we have to call the function once to get the graph to draw for the first time. This change is necessary because we need access to the function in the onClick() function as well, and the immediate invocation syntax does not allow access to the function name outside of the function. When the user clicks the graph, we need to kick start the animation again by calling animate() after changing t:

  function onClick() {
    if (offset == 0) offset = 2;
    else if (offset == 2) offset = -2;
    else offset = 0;
    sineText.text = 'f(t) = sin(t) + ' + offset;
    animate();
  }

It's too bad that we have to call animate() explicitly again, but the improvement in processor usage is worth it. Animation is processor intensive, and there's no reason for graphs to be taking up processor time and adversely impacting other graphs when they're not changing at all.

These same changes can be applied to the other graph code in this post, and then they all look pretty good, so after committing the small addition we made to the API, we can move on to the next post that covers sampling.

Extracting Sampling Graph Primitives

In the post on DSP sampling, there are two graphing primitives that we can add to our API feature set. The first one is a primitive that draws little sample circles on a moving sine curve, and the second one is a primitive that draws an animated blip on a moving sine curve as it passes a certain point. These animation features can be seen in the following graph:

The code for the animation of this graph is as follows:

  (function animate() {
    t += 0.04;
    if (t > 20) t = 0;

    sinewave.clear();
    dsp_graph.drawSine(sinewave, 0x888888, amplitude, offset, freq, t, 0, 10);
    var y = amplitude*Math.sin((10*freq + t)/10.0*Math.PI);
    sinewave.lineStyle(2/26.0, 0xeeeeee, 1);
    sinewave.moveTo(10,0);
    sinewave.lineTo(10,y);

    // Draw the sampling blip
    sinewave.drawCircle(10,y,2/26.0);
    var sample_offset = (t/2 - Math.floor(t/2))*2;
    if (sample_offset <= 0.3) {
      y = amplitude*Math.sin((10*freq + t - sample_offset)/10.0*Math.PI);
      sinewave.drawCircle(10 - sample_offset,y,sample_offset);
    }

    dsp_graph.drawSine(sinewave, 0x00bbdd, amplitude, offset, freq, t, 10, 20);

    // Draw the sample circles
    for (var x = 2 - sample_offset; x <= 10; x+=2) {
      y = amplitude*Math.sin((x*freq + t)/10.0*Math.PI);
      sinewave.drawCircle(x, y, 4/26.0);
    }

    renderer.render(stage);
    requestAnimationFrame( animate );
  }());

The rest of the code for initializing the graph should be familiar by now, so I omitted it. We can easily extract functions for each of those two primitives into our API so we can reuse them in the rest of the graphs in this post, and possibly any later posts where I overlaid samples on a curve. The API functions look like this:

    drawSamples: function(sinewave, amplitude, freq, phase, start, end, step) {
      for (var x = start; x <= end; x+=step) {
        var y = amplitude*Math.sin((x*freq + phase)/UNIT_FREQ*Math.PI);
        sinewave.drawCircle(x, y, 4/TICK_STEP);
      }
    },

    drawSampleBlip: function(sinewave, amplitude, freq, phase, sample_offset, x) {
      var y = amplitude*Math.sin((x*freq + phase)/UNIT_FREQ*Math.PI);
      sinewave.drawCircle(x,y,2/TICK_STEP);
      if (sample_offset <= 0.3) {
        y = amplitude*Math.sin((x*freq + phase - sample_offset)/UNIT_FREQ*Math.PI);
        sinewave.drawCircle(x - sample_offset, y, sample_offset);
      }
    },

These functions are basically a direct copy of the code that was creating the graph features before, with all of the required parameters added to the function parameter list. I also added another constant called UNIT_FREQ to define 10 as the frequency scalar that represents a frequency of one. That constant was looking more necessary now that it has been used three times in the API. The addition of these primitives to the API allows us to clean up the animate() function in that first graph a bit:

  (function animate() {
    t += 0.04;
    if (t > 20) t = 0;

    sinewave.clear();
    dsp_graph.drawSine(sinewave, 0x888888, amplitude, offset, freq, t, 0, 10);
    var y = amplitude*Math.sin((10*freq + t)/10.0*Math.PI);
    sinewave.lineStyle(2/26.0, 0xeeeeee, 1);
    sinewave.moveTo(10,0);
    sinewave.lineTo(10,y);

    var sample_offset = (t/2 - Math.floor(t/2))*2;
    dsp_graph.drawSampleBlip(sinewave, amplitude, freq, t, sample_offset, 10);
    dsp_graph.drawSine(sinewave, 0x00bbdd, amplitude, offset, freq, t, 10, 20);
    dsp_graph.drawSamples(sinewave, amplitude, freq, t, 2 - sample_offset, 10, 2);

    renderer.render(stage);
    requestAnimationFrame( animate );
  }());

And we can now use these primitives to clean up the rest of the graphs in the sampling post. A couple of the later graphs in the post have some more complex curves and animations in them, but they are only used once in the whole series of posts so it's probably not worth refactoring them. We can call this post done, commit our additions to the API, and try to knock off one more set of graphs before calling it quits.

Cleaning up the Averaging Post

At this point it should be clear that most of the refactoring just involves identifying functions that can be extracted into the API, and tweaking them a bit to work generally for a wider variety of graphs. We're looking for primitive graph features that we can stick in a commonly accessible place, and then use these primitives in different combinations to create the different animated graphs with less code. The graphs in the averaging post are no different. I even had started extracting functions in the code for this post when I first wrote it, but I didn't pull them all the way into the API module. I lazily left them in each graph's drawing code and then copied them from one graph to another along with all of the rest of the code. That was bad and probably cost me time in the long run, but we're going to fix it now. Here's the code for the first average graph as it stands after the refactorings up to this point:

$(function() {
  var renderer = dsp_graph.initCanvas('#canvas-gas-average', onClick);
  var stage = new PIXI.Container();

  var x_labels = ['9/1997','9/1999','9/2001','9/2003','9/2005','9/2007','9/2009','9/2011','9/2013']
  var y_labels = ['$1','$2','$3','$4','$5']
  dsp_graph.drawPositiveAxis(stage, x_labels, y_labels);

  function Mean(ary) {
    return ary.reduce(function(a, b) { return a + b; }) / ary.length;
  }

  var t = 0;
  var state = 0;
  var avg = Mean(gas_prices);
  var avg_gas_prices = gas_prices.slice();

  var graph = dsp_graph.createCurve(stage);
  graph.lineStyle(2.0/26.0, 0x888888, 1);
  drawPoints(graph, gas_prices);

  var avg_graph = dsp_graph.createCurve(stage);

  function onClick() {
    if (state == 0) state = 1;
    else state = 0;
    animate();
  }

  function drawPoints(graphics, ary) {
    var x = 0;
    var step = 20.0 / ary.length;
    graphics.moveTo(x, ary[0]*2);
    ary.forEach(function(y) {
      graphics.lineTo(x, y*2);
      graphics.moveTo(x, y*2);
      x += step;
    })
  }

  function animate() {
    if (state == 1) {
      t += 0.01;
      if (t < 1) {
        avg_gas_prices = gas_prices.map(function(price) { return price - t*(price - avg) })
      } else {
        avg_gas_prices = avg_gas_prices.fill(avg)
      }
    } else {
      if (t > 0) t = 0;
      avg_gas_prices = gas_prices.slice();
    }

    avg_graph.clear();
    avg_graph.lineStyle(2/26.0, 0x00bbdd, 1);
    drawPoints(avg_graph, avg_gas_prices);

    renderer.render(stage);
    console.log("animate()")
    if (state == 1 && t < 1.2) requestAnimationFrame( animate );
  }

  animate();
});

It's actually not too bad, but the Mean() and drawPoints() functions should definitely be moved to the API. The Mean() is easy:

    mean: function(ary) {
      return ary.reduce(function(a, b) { return a + b; }) / ary.length;
    },

The drawPoints() function is only slightly more complicated because we're going to rename the parameters to be more descriptive and add in the line style that we're going to use to draw the curve:

    drawYPoints: function(curve, color, points) {
      curve.lineStyle(2.0/TICK_STEP, color, 1);
      var x = 0;
      var step = 20.0 / points.length;
      curve.moveTo(x, points[0]*2);
      points.forEach(function(y) {
        curve.lineTo(x, y*2);
        curve.moveTo(x, y*2);
        x += step;
      })
    },

This function is very similar to another function in the API called drawCurve(), but that function expects an object with 'x' and 'y' keys, while drawYPoints() takes only an array of y-values. We could try combining them, but that would require extra code to handle the conversion from one form to the other, or extra code at the call site to get the arguments into the right form for the function. Everything is cleaner if we keep the two functions separate, and they're more descriptive that way, too. The refactored code basically eliminated those two local function definitions and a couple other lines of line styling code:

$(function() {
  var renderer = dsp_graph.initCanvas('#canvas-gas-average', onClick);
  var stage = new PIXI.Container();

  var x_labels = ['9/1997','9/1999','9/2001','9/2003','9/2005','9/2007','9/2009','9/2011','9/2013']
  var y_labels = ['$1','$2','$3','$4','$5']
  dsp_graph.drawPositiveAxis(stage, x_labels, y_labels);

  var t = 0;
  var state = 0;
  var avg = dsp_graph.mean(gas_prices);
  var avg_gas_prices = gas_prices.slice();

  var graph = dsp_graph.createCurve(stage);
  dsp_graph.drawYPoints(graph, 0x888888, gas_prices);

  var avg_graph = dsp_graph.createCurve(stage);

  function onClick() {
    if (state == 0) state = 1;
    else state = 0;
    animate();
  }

  function animate() {
    if (state == 1) {
      t += 0.01;
      if (t < 1) {
        avg_gas_prices = gas_prices.map(function(price) { return price - t*(price - avg) })
      } else {
        avg_gas_prices = avg_gas_prices.fill(avg)
      }
    } else {
      if (t > 0) t = 0;
      avg_gas_prices = gas_prices.slice();
    }

    avg_graph.clear();
    dsp_graph.drawYPoints(avg_graph, 0x00bbdd, avg_gas_prices);

    renderer.render(stage);
    console.log("animate()")
    if (state == 1 && t < 1.2) requestAnimationFrame( animate );
  }

  animate();
});

The rest of the graph code in this post can be refactored in a similar way. The last graph on averaging with an FIR filter has an additional pair of functions that should be extracted into the API, and I'll show them here for completeness. The process of putting them into the API and calling them from the code is the same as before:

    filter: function(ary, taps) {
      if (ary.length < taps.length) {
        var gain = taps.slice(0, ary.length).reduce(function(a, b) { return a + b; })
        taps = taps.map(function(a) { return a / gain })
      }
      return ary.reduce(function(a, b, i) { return a + b*taps[i] }, 0)
    },

    genFilter: function(n) {
      var taps = []
      for (var i = 0.5-n/2.0; i < n/2.0; i+=1.0) {
        taps.push(Math.sin(Math.PI*i/26.0) / (Math.PI*i/26.0))
      }
      var gain = taps.reduce(function(a, b) { return a + b; })
      taps = taps.map(function(a) { return a / gain })
      return taps
    },

That about wraps up the graphs in the post on averaging. After committing the DSP graphing changes, we can look back and see that we fairly quickly walked through three posts worth of graphs, cleaning things up nicely along the way. At this point, most of the refactorings in the rest of DSP series posts will be fairly similar. We already did most of the hard work of fixing bugs, improving performance, and getting the API into a state where it became much more useful. Now it's relatively easy to remove the rest of the duplication and finish cleaning up. That's what a lot of refactoring is, just getting the code organized, making it more clear and readable, and essentially adding a good dose of sanity to an otherwise muddled mess.

The earlier and the better you refactor your code, the easier it is to write more code and make faster progress. I've learned those lessons over again while doing this series of posts, and I'm kicking myself for not doing most of these refactorings while I was writing the original DSP series. I would have had a much easier time of it, and I wouldn't have spent so much time copy-and-pasting and doing little tweaks to code that was very similar from one graph to the next. When I was in the middle of it, I didn't want to take the time to clean things up; I just wanted to get each graph done and move onto the next one. But I paid for it in a messy code base, and lots of wasted time trudging through hundreds of lines of repetitive code. Don't make the same mistake. Refactor early, and well.