A simple refactoring

Refactoring is the act of taking a function, a method, a class or an entire application and modify it without altering its functionalities in order to make it better, more performant, more mantainable. To improve it in some way while mantining its original behavior.

I recently worked on the Metric/Imperial converter project on freecodecamp.org and had the opportunity to make a little refactoring.

I'd like to write down and share the passages that led from the initial implementation to the, hopefully better, last form.

I'm not claiming I've done a mirable work or I'm revealing some hidden truth. I'm simply trying to elaborate my thoughts on coding, share them and get feedbacks!

So, let's dive in 🏊‍♂️

You can find the specification on the project page, but the bit we are interested in is the following:

I can use fractions, decimals or both in my parameter (ie. 5, 1/2, 2.5/6), but if nothing is provided it will default to 1.

The input string should also include the unit we are converting from, so valid inputs are:

4gal
1/2km
5.4/3lbs
kg

The project blueprints make us implement a getNum(input) function whose goal is to parse the numeric part of the input. That's the function we will focus on.

Refactoring would not be possible without a suite of tests that guarantees we are not altering the behavior of our code and potentially adding bugs.

So here it is!

    test('Whole number input', function(done) {
      var input = '32L';
      assert.equal(convertHandler.getNum(input),32);
      done();
    });

    test('Decimal Input', function(done) {
      var input = '3.2L';
      assert.equal(convertHandler.getNum(input),3.2);
      done();
    });

    test('Fractional Input', function(done) {
      var input = '3\/2L';
      assert.equal(convertHandler.getNum(input),1.5);
      done();
    });

    test('Fractional Input w/ Decimal', function(done) {
      var input = '3.5\/2L';
      assert.equal(convertHandler.getNum(input),1.75);
      done();
    });

    test('Invalid Input (double fraction)', function(done) {
      var input = '3\/4\/2L';
      assert.throws(convertHandler.getNum.bind(convertHandler.getNum, input), 'invalid number');
      done();
    });

    test('No Numerical Input', function(done) {
      var input = 'L';
      assert.equal(convertHandler.getNum(input),1);
      done();
    });

Here we are adhering to the tests provided by the project. More cases and conditions could be tested but let's keep it simple.

My (really naive) first implementation is the following:

function ConvertHandler() {

  this.getNum = function(input) {
    var result;
    let match = /[a-zA-Z]/.exec(input); // Searching for the unit part
    if (match) {
      result = input.substring(0, match.index);
    }

    if (!result) {
      return 1;
    }

    if (result.indexOf('/') != -1) { // Parsing the fraction
      let operands = result.split('/');

      if (operands.length != 2) {
        console.log('throwing error');
        throw new Error('invalid number');
      }
      result = parseFloat(operands[0]) / parseFloat(operands[1]);
    }
    else if (result.indexOf('.') != -1) {
      result = parseInt(input);
    } else {
      result = parseFloat(input);
    }

    return result;
  };
}

It hurts the eyes (and I wrote it! 😖) and, though it adheres to the specifications and passes the tests, it's not really clear what's going on. Morever, modifying it when new requirements show up would not be trivial.

I wrote it following the requirements driven by tests. So, I added "features" in the order they were presented by the tests.
In retrospective, I think this approach quite mimics what generally happens on a codebase when new requirements pop out: the requirement is analyzed, you find a suitable spot to implement it, write some code trying to adapt to what is already there and make it work.

And that's perfectly fine, but once it works, we should take some time to reflect on what's going on and if there's any room for improvement (spoiler: there always is). It's not easy and it requires a lot of effort. In that sense, I think this kind of exercise is really useful for building a sort of "muscle memory" on refactoring.

In this specific case, it came to my mind that integers and decimal numbers are merely special case of a fraction.
So, from a functional point of view, we can generalize the method so that it handles fractions only.

We should only provide sensible defaults for numerator and denominator:

• denominator = 1 when there is only an operand
• numerator = denominator = 1 when no number is provided

So, let's try a second iteration

function ConvertHandler() {

  this.getNum = function(input) {
    var result;
    let match = /[a-zA-Z]/.exec(input); // Searching for the unit

    if (match) {
      result = input.substring(0, match.index);
    } else {
      throw new Error('invalid input');
    }

    let numerator;
    let denominator;
    let operands = result.split('/'); // Parsing the fraction

    if (operands.length > 2) {
      throw new Error('invalid number');
    }

    if (operands.length >= 1) {
      numerator = parseFloat(operands[0]);
    }

    if (operands.length == 2) {
      denominator = parseFloat(operands[1]);
    }

    result = (numerator||1) / (denominator||1)

    return result;
  };
}

Much better! 😃

The function now tries to parse a fraction by splitting on '/', check for how many operands are provided and applies the defaults by short-circuiting the variables (e.g. numerator||1)

Now we have cleared our mind on the function even the code turned out clearer: variables have more meaningful names, flow control has less branching and in general is easier to read the code.
The test suite guarantees that the behavior of the function is the same.

The function is still a little verbose, with lot of if statements and some room for inlining. We can take advantage of some features of the language to make the code more concise.

For example, we can leverage the fact that javascript won't complain if we access an array out of bounds, returning an undefined value instead:

function ConvertHandler() {

  this.getNum = function(input) {
    let match = /[a-zA-Z]/.exec(input); // Searching for the unit
    let numericString 

    if (match) {
      numericString = input.substring(0, match.index);
    } else {
      throw new Error('invalid input');
    }

    let operands = numericString.split('/'); // Parsing the fraction

    if (operands.length > 2) {
      throw new Error('invalid number');
    }

    return (parseFloat(operands[0]) || 1) / (parseFloat(operands[1]) || 1);
  };
}

Here I also inlined the parseFloat() invocation since I find there is no value in keeping the two variables for numerator and denominator.

One thing that really bothers me at this point, is the presence of operations like regex matching and string parsing. They are a little too low level and require a lot of brain power to figure out what their purpose is; also, the accompanying comments are a hint that something should be done to improve readability and comprehension of the code.

One technique that tackles this kind of problem is method extraction: we literally take pieces of our code and encapsulate them in external functions that we could invoke in place of the removed code.
So we can reason to a higher level, with the added benefit that we can name our functions in a more meaningful way, thus conveying the real intention of our code.

In this iteration I extracted the findUnitIndex(), extractOperands() and parseAndCoalesce() methods.

function ConvertHandler() {

  this.getNum = function(input) {
    const unitIndex = findUnitIndex(input);
    const operands = extractOperands(input, unitIndex);
    return parseAndCoalesce(operands[0]) / parseAndCoalesce(operands[1]);
  };

  /*
   * Extracted methods
   */
  function findUnitIndex(input) {
    const match = /[a-zA-Z]/.exec(input);
    if (!match) {
      throw new Error('invalid input');
    }

    return match.index;
  }

  function extractOperands(input, matchIndex) {
    const operands = input.substring(0, matchIndex).split('/');
    if (operands.length > 2) {
      throw new Error('invalid number');
    }

    return operands;
  }

  function parseAndCoalesce(operand) {
    return parseFloat(operand) || 1
  }
}

The resulting code in the main function is more concise and it's really easy to understand what's going on at high level.
Complexity is pushed down in the extracted methods, so we really didn't get rid of it. But we have isolated and "labelled" it, making it easier to intervene.

One last thing that I'd like to change is the line:

parseAndCoalesce(operands[0]) / parseAndCoalesce(operands[1]);

to make its purpose clearer.

What I came up to is a brand new concept: CoalescingFraction (I don't really know if that'a thing but I think we can get creative here).
The idea is that of a fraction that defaults its numerator and denominator to 1 in case they are not provided (it would have made more sense that numerator defaulted to 0, but we are following the project specification).
The technique used here is class extraction: we encapsulated an entire concept in a new class, pushing it away from our main code and also making it available to other parts of our application.

function ConvertHandler() {

  this.getNum = function(input) {
    const unitIndex = findUnitIndex(input);
    const operands = extractOperands(input, unitIndex);
    return new CoalescingFraction(operands[0], operands[1]).value();
  };

  /*
   * Extracted methods
   */

   // as previous step; redacted for readability

}

/*
 * Extracted class
 */
function CoalescingFraction(numerator, denominator) {

  this.value = function() {
    return parseAndCoalesce(numerator) / parseAndCoalesce(denominator);
  }

  function parseAndCoalesce(value) {
    return parseFloat(value) || 1
  }
}

More inlining is possible, but I think that is good enough.

Conclusions

Refactoring is something I really should do more often and this kind of little exercises are a good way to make practice and explore the possibilities in a safe environment.

Tests are important in order to guarantee the correct outcome. However, I think one should find the right granularity: having too many of them to mantain could indeed make refactoring harder.

When resolving a problem, we often stop once a solution is found. Taking some time to think harder on it may lead to better results that are both more effective and pleasant to our mind.

Hope you enjoyed the post! 😊