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

I've been on a refactoring kick lately, and last time I started refactoring some fairly recent code I wrote for my Everyday DSP for Programmers series. I was able to fix a long-standing issue with touch screen support and refactor the API that I had started for drawing animated graphs on the HTML5 canvas. Now that I have a decent foundation, it's time to start systematically walking through the code in the blog posts and extracting the parts that are repetitive into the API so that the code left in each blog post can be lean and clean.

Cleaning up the Basic DC Signal

Let's take a look at the code i wrote for the very first graph of a basic DC signal:

$(function() {
  var renderer = dsp_graph.initCanvas('#canvas-dc', null);

  var stage = new PIXI.Container();
  dsp_graph.drawPositiveAxis(stage);

  var graphics = new PIXI.Graphics();
  graphics.lineStyle(2, 0x00bbdd, 1);
  graphics.moveTo(15, 150);
  graphics.lineTo(535, 150);
  stage.addChild(graphics);

  animate();

  function animate() {
    renderer.render(stage);
  }
});

First, because it's a DC signal, this graph does not change. It's obvious in the animate() function that nothing is happening, and this function is useless. In later graphs this function will be very important, but that doesn't mean I should include it here where it does nothing. It should be nixed. The call to renderer.render(stage) needs to still happen to draw the graph once, but it's fine to move it out of animate().

The next issue is that I've used a bunch of magic numbers in drawing the DC curve (a straight line, really). It looks innocent enough when it's just a few lines with numbers in them in a small function, but these numbers hide a problem that will continue to grow as the graphs get more complicated. There's a coordinate offset hidden in these numbers because of the margins outside of the graph axes, and things will be much simpler later if I create an origin for the axes and a set graph unit size that can be used throughout the drawing code to draw curves on axes with an origin of (0, 0). Luckily, any decent graphics library has a transform function, and PixiJS is no exception. I can set up a transform on the graphics object that is used to draw the DC curve:

$(function() {
  var renderer = dsp_graph.initCanvas('#canvas-dc', null);

  var stage = new PIXI.Container();
  dsp_graph.drawPositiveAxis(stage);

  var graphics = new PIXI.Graphics();
  stage.addChild(graphics);
  graphics.setTransform(15, 280, 26, -26);
  graphics.lineStyle(2/26.0, 0x00bbdd, 1);
  graphics.moveTo(0, 5);
  graphics.lineTo(20, 5);

  renderer.render(stage);
});

The first two parameters of setTransform() are the X and Y offsets. The next two parameters are the X and Y scale factors that set the unit size for the graph. The negative value for the Y scale factor is necessary to flip the Y axis. Normally, as the Y value increases, the point location will go down the canvas, but we want the opposite behavior so that it makes more sense for drawing on the graph axes. We also need to be sure to later scale down the line width and points because of the scale change in the transform. This may not look like much of an improvement, yet, but bear with me. It's worth it. The next thing we're going to do is extract the creation of a curve object into the API module:

    createCurve: function(stage) {
      var curve = new PIXI.Graphics();
      stage.addChild(curve);
      curve.setTransform(15, 280, 26, -26);
      return curve;
    },

This function will create a graphics object with the right transform parameters, and add it to the stage that's provided to the function. As is, this setup will work for the single positive axis that we're drawing on for the DC signal, but when we start using other axis types, the offset will be wrong for the origin. What we need to do is somehow let createCurve() know which axis type we're using so that it can set the right origin. Better yet, we can figure out a way to tell it what the origin should be directly. Each axis drawing function knows what it's origin should be, and the stage is passed to both the axis drawing functions and the createCurve() function, so we can set an origin in the stage object in each axis drawing function like this for drawPositiveAxis():

      stage.origin = {x: X_AXIS_START, y: Y_AXIS_END};

And then use the origin in createCurve():

    createCurve: function(stage) {
      var curve = new PIXI.Graphics();
      stage.addChild(curve);
      curve.setTransform(stage.origin.x, stage.origin.y, TICK_STEP, -TICK_STEP);
      return curve;
    },

Notice that I also changed the scale factors to use the constants for the axis tick step, which will be our unit size. After this clean up, all we have left is the three lines of code that set the line style and draw the line. We can extract that into a drawCurve() function that we'll extend later as we draw more complex curves:

    drawCurve: function(curve, color, points) {
      curve.lineStyle(2.0/TICK_STEP, color, 1);
      curve.moveTo(points.x[0], points.y[0]);
      for (var i = 1; i < points.x.length; i++) {
        curve.lineTo(points.x[i], points.y[i]);
      }
    },

This function takes the graphics context, called curve, the color to draw the curve, and the points as a hash of x- and y-values. Using it is pretty straightforward, and with it the DC signal graph code has been cleaned up quite nicely:

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

  dsp_graph.drawPositiveAxis(stage);

  var dc_signal = dsp_graph.createCurve(stage);
  dsp_graph.drawCurve(dc_signal, 0x00bbdd, {x:[0, 20], y:[5, 5]});

  renderer.render(stage);
});

We initialize the canvas and create a stage, draw the axis, create and draw a curve, and then render the whole shebang. This code reads much better than it did before, and we've created some useful functions that we'll reuse throughout the rest of the graphs. It's time to commit the changes we made to the API module and move on to the next graph.

Cleaning up Animation

The next graph, the impulse function, is the first animated graph, so we'll have some additional things to refactor to try to make setting up an animation a little cleaner. In the interest of brevity, I'll show each new graph code with the changes from previous refactorings already applied. There's no need to be verbose about API improvements that we've just gone over, and it would take forever to show them anyway. As we add more functionality to the API, it should get easier and easier to clean up the later graph code. Here's the updated code for the animated impulse graph:

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

  dsp_graph.drawPositiveAxis(stage);

  var impulse = dsp_graph.createCurve(stage);

  var t = 20;

  animate();

  function animate() {
    t -= 0.08;
    if (t < 0) t = 20;

    impulse.clear();
    dsp_graph.drawCurve(impulse, 0x00bbdd, {x:[0, t, t, t, 20], y:[1, 1, 9, 1, 1]})

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

This code already looks pretty clean and simple, but there's at least one thing we can improve. The animation is done by defining a function that is called for each frame of the animation. A tick variable t is defined to keep track of how much the graph should move as we step through the frames, and this function also calls requestAnimationFrame() to, you guessed it, request the next frame in the animation. To kick this all off, we need to call animate() the first time after everything is initialized. Instead of calling it explicitly, we can call the function immediately after defining it, like so:

  (function animate() {
    t -= 0.08;
    if (t < 0) t = 20;

    impulse.clear();
    dsp_graph.drawCurve(impulse, 0x00bbdd, {x:[0, t, t, t, 20], y:[1, 1, 9, 1, 1]})

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

It's a small improvement, but I'll take it. The same thing can be done to every other piece of animated graph code, and I'll do it automatically from now on.

Next Up, the Sine Function

The next graph of the step function doesn't show anything new, so let's move on to the graph of the sine function. Here's what the code looks like with the latest improvements:

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

  dsp_graph.drawZeroAxis(stage);

  var sinewave = dsp_graph.createCurve(stage);

  var t = 20;
  var amplitude = 4;

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

    sinewave.clear();
    sinewave.lineStyle(2/26.0, 0x00bbdd, 1);

    var y = amplitude*Math.sin((t)/10.0*Math.PI);
    sinewave.moveTo(0, y);
    for (var x = 0.5; x <= 20; x+=0.5) {
      y = amplitude*Math.sin((x+t)/10.0*Math.PI);
      sinewave.lineTo(x, y);
    }
    renderer.render(stage);
    requestAnimationFrame( animate );
  }());
});

The chunk of code in animate() that draws the sine wave is reused multiple times in the next couple articles in the series, and I even created a more generic function for it in the API module, although I didn't call this function in the above code:

    drawSine: function(sinewave, amplitude, offset, freq, phase, start, end) {
      start = typeof start != 'undefined' ? start : 0
      end = typeof end != 'undefined' ? end : 520
      var step = freq > 0.1 ? 12/freq : (freq < -0.1 ? 12/-freq : 104);
      var y = amplitude*Math.sin((start*freq + phase)/260.0*Math.PI) + offset;
      sinewave.moveTo(start, -y);
      for (var x = start; x < end; x+=step) {
        y = amplitude*Math.sin((x*freq + phase)/260.0*Math.PI) + offset;
        sinewave.lineTo(x, -y);
        sinewave.moveTo(x, -y);
      }
      y = amplitude*Math.sin((end*freq + phase)/260.0*Math.PI) + offset;
      sinewave.lineTo(end, -y);
    },

This function still has some magic numbers, and these numbers are no longer scaled correctly for how we're scaling the graph coordinate system. That needs to be fixed. We want to use something like this, but it would be more convenient to have a function that generates the array of points that we can then send to drawCurve() to do the actual drawing part. That arrangement separates the calculation of the sine wave points from the drawing of the points, potentially making both functions more useful. Let's make a new function called generateSine() based on drawSine():

    generateSine: function(amplitude, offset, freq, phase, start, end) {
      var step = Math.abs(freq) > 0.1 ? 0.5/Math.abs(freq) : 5;
      var points = {x:[],y:[]};
      for (var x = start; x <= end; x+=step) {
        points.x.push(x);
        points.y.push(amplitude*Math.sin((x*freq + phase)/10.0*Math.PI) + offset);
      }
      return points;
    },

This function focuses on generating the points for the sine wave, and it is much cleaner as a result. The first line of the function is there to make sure that the step size for the x-dimension is the right size to make the curve look reasonably smooth. It's been cleaned up a bit from the mess that was the step variable initialization in drawSine(). Now that we have this function, we can use it and drawCurve() in drawSine() to make it much simpler:

    drawSine: function(sinewave, color, amplitude, offset, freq, phase, start, end) {
      start = typeof start != 'undefined' ? start : 0
      end = typeof end != 'undefined' ? end : 20
      var points = this.generateSine(amplitude, offset, freq, phase, start, end);
      this.drawCurve(sinewave, color, points);
    },

I decided to leave a few of the magic numbers in there because, frankly, I don't know what to do with them. I could make them named constants, but they're only used within these functions so that wouldn't significantly improve the code. I could make them part of the interface so that the graphs would be more flexible, but all of my graphs are the same size and doing that complicates the interface unnecessarily while requiring that I duplicate those numbers outside these functions where ever I call them. If I was writing a more generic graphing API, I would definitely do something about the leftover magic numbers, but I'm not, so I'll leave them as they are.

The last graph in this post has been nicely simplified with this refactoring:

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

  dsp_graph.drawZeroAxis(stage);

  var sinewave = dsp_graph.createCurve(stage);

  var t = 20;

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

    sinewave.clear();
    dsp_graph.drawSine(sinewave, 0x00bbdd, 4, 0, 1, t);

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

With that done, we'll stop for now and commit these changes. Next time, we'll take a look at refactoring the next few posts in the series, and probably pick up the pace quite a bit because the API will be reaching a point where most of the code simplifies easily.