Roman to Arabic numerals converter in Javascript

Question

I was given some homework as a technical test by a company I wanted to work for... Which I failed to the point of not being able to even access the code review portion of the interviewing process.

Their feedback was : The team had said because the numerals were converted to integers and there were lots of magic numbers in different places, it was hard to follow.

But I really do not understand what it means, in regards to my code. Can someone please explain, what this means specifically, in terms of code ?

Here is an example of the expected behaviour:

var romanNumber1 = new RomanNumber('XX');
var romanNumber2 = new RomanNumber(40);
console.log(romanNumber1.toInt()); // => 20
console.log(romanNumber1.toString()); // => 'XX'
console.log(romanNumber2.toInt()); // => 40
console.log(romanNumber2.toString()); // => ‘XL’

Also, it was specified that if the library is called as a function (i.e. without the new prefix), I should ensure I still pass back a new object. It was also specified that I should throw exceptions and also, what exceptions and in which case.

So my solution works and passed all the extensive tests I wrote, but as I said earlier, it was deemed hard to follow, and I really would like to understand why to ensure if doesn't happen again.

Here is my solution :

const FAILURE = -1
const ROMAN = 1
const ARABIC = 2

const VALUES = {
  'M' : 1000,
  'D' : 500,
  'C' : 100,
  'L' : 50,
  'X' : 10,
  'V' : 5,
  'I' : 1
};
const KEY_ENTRIES = Object.keys(VALUES)
const VALUE_ENTRIES = KEY_ENTRIES.map(k=> VALUES[k]);

function RomanNumber(value) {

  /*
  **  _getType(value)
  ** Returns numeral type, FAILURE, ROMAN or ARABIC,
  ** Throws exceptions if invalid
  */
  this._getType =  function() {
    let value = this._value;

    if (value == null || (typeof value == 'string' && !value)) {
        this._type = FAILURE;
        throw("value required");
      }
    if (typeof value == 'string') {
      if (/^\-?[0-9]*$/.test(value)) {
        value = parseInt(value)
        if (value < 1 || value > 3999) {
          this._type = FAILURE;
          throw("invalid range");
        }
        this._value = value;
        this._type = ARABIC;
      }
      else if (/^([M|D|C|L|X|V|I])*$/.test(value)) {
        if (this._checkRomanIsValid(value))
          this._type = ROMAN;
        else {
            this._type = FAILURE;
            throw("invalid value");
        }
      }
      else {
        this._type = FAILURE;
        throw("invalid value");
      }
    }

    else if (typeof value == 'number') {
      if (value < 1 || value > 3999) {
        this._type = FAILURE;
        throw("invalid range");
      }
      if (value % 1) {
          this._type = FAILURE;
          throw("invalid value");
      }
      this._type = ARABIC;
    }
    return this._type;
  }

  /*
  **  _checkRomanIsValid(str)
  ** Checks if the roman numeral string is valid,
  ** eg: IIX is valid, while IIIIX is not
  */
  this._checkRomanIsValid = function(str) {
      let count = 0;
      let last = null;
      for (var i = 0; i < str.length; i++) {
        if (last != null && last == str[i])
          count += 1;
        else if (last != null)
          count = 0;
        if (count >= 3)
          return false;
        last = str[i];
      }
      return true;
  }

  /*
  **  _buildArrayFromStr(value)
  ** Builds a value array from a roman numeral string
  ** eg MMMDXXVII =>  [1000,1000,1000,500,10,10,5,1,1]
  */
  this._buildArrayFromStr = function() {
      let last = null;
      let res = [];
      for (var i = 0; i < this._value.length; i++) {
        if (last != null && last < VALUES[this._value[i]])
            res[res.length - 1] = VALUES[this._value[i]] - last;
        else
          res.push(VALUES[this._value[i]]);
        last = VALUES[this._value[i]];
      }
      this._valueArray = res;
  }
  /*
  **  _buildArrayFromNb(value)
  ** Builds a value array from an arabic number
  ** eg 1992 =>  [1000, 100, 1000, 10, 100, 1, 1]
  */
  this._buildArrayFromNb = function() {
      let res = [];
      let str = this._value.toString();
      let len = str.length - 1;
      for (var i = 0; i < str.length; i++) {
        let pow = 1;
        for (var j = 0; j < len; j++)
          pow *= 10;

        if ((str[i] >= 1 && str[i] < 4)) {
          for (var k = 0; k < str[i]; k++)
            res.push(pow);
        }
        else if ((str[i] == 4)) {
            res.push(pow);
            res.push(pow / 2 * 10);
        }
        else if ((str[i] == 5)) {
            res.push(pow / 2 * 10);
        }
        else if ((str[i] >= 6 && str[i] < 9)) {
          res.push(pow / 2 * 10);
          for (var k = 0; k < str[i] - 5; k++)
            res.push(pow);
        }
        else if ((str[i] == 9)) {
            res.push(pow);
            res.push(pow * 10);
        }
        len -= 1;
      }
      this._valueArray = res;
  }

  /*
  **  toString()
  ** Returns the roman numeral value
  */
  this.toString = function() {
    if (this._type == FAILURE)
      return FAILURE;
    if (this._type == ROMAN)
      return this._value;
    this._buildArrayFromNb(this._value);
    let res = "";
    this._valueArray.forEach(function(elem){
      res += KEY_ENTRIES[VALUE_ENTRIES.indexOf(elem)];
    });
    return res;
  }

  /*
  **  toInt()
  ** Returns the arabic numeral value
  */
  this.toInt = function() {
    if (this._type == FAILURE)
      return FAILURE;
    if (this._type == ARABIC)
      return this._value;
    this._buildArrayFromStr(this._value);
    let res = 0;
    this._valueArray.forEach(function(elem){
      res += elem;
    });
    return res;
  }




  /*
  ** getters/setters
  */
  this.setValue = function(value) {
    this._value = value;
  }


  /*
  ** ctor : RomanNumber(value)
  */
  this._value = value;
  if (value)
    this._type = this._getType();
}

module.exports = function(value) {
  return new RomanNumber(value);
};

Answer 1

You wrote way too much code. Interviewers hate wading through a lot of code. Your solution was probably rejected just based on the poor impression given by its length.

The specification, which requires the same object to support conversions in both directions, is a bit weird for my taste. Nevertheless, I think that the way you wrote all that type-detection and validation code in _getType() is overkill. If the exercise does not specify how to handle out-of-range or illegal values, it's fair game to consider it undefined behaviour, and do the simplest thing possible.

The specification does not call for a .setValue() mutator, so you shouldn't write one.

I agree with their assessment that magic numbers shouldn't be hard-coded all over the place. You created VALUES as a lookup table, which was good. However, in _getType(), you also have a /^\-?[0-9]*$/ and /^([M|D|C|L|X|V|I])*$/ validation regexes. (The latter regex demonstrates a lack of understanding of how character classes and capturing parentheses work: it should just be /^[MDCLXVI]+$/.) Even worse, your _buildArrayFromNb() is rather convoluted, and has cases based on magic numbers 1, 4, 5, 6, 9.

Finally, the algorithms seem too complicated. Both conversions can be done without transforming the input to an intermediate array representation.

Your use of the term "Arabic" is technically incorrect. This object converts between Roman numerals and integers. 40 is a number; the string '40' would be its Arabic numeral representation.

Suggested solution

These conversion algorithms are completely driven by lookup tables. If desired, you could reintroduce some validation by throwing an exception on failed lookups during the conversion process.

function RomanNumber(value) {
  if (typeof value === 'string') {
    let num = parse(value);
    this.toInt = function() { return num; }
    this.toString = function() { return value; }
  } else if (typeof value === 'number') {
    let str = format(value);
    this.toInt = function() { return value; }
    this.toString = function() { return str; }
  } else {
    throw 'Invalid value';
  }
  
  function parse(str) {
    const VALUES = {
                  'M' : 1000,
      'D' :  500, 'C' :  100,
      'L' :   50, 'X' :   10,
      'V' :    5, 'I' :    1,
    };

    let prev = 0, sum = 0;
    for (let i = 0; i < str.length; i++) {
      let curr = VALUES[str[i]];
      sum += (prev < curr) ? curr - 2 * prev
                           : curr;
      prev = curr;
    }
    return sum;
  }
  
  function format(num) {
    const LETTERS = [
                                               [1000, 'M'],
      [ 900, 'CM'], [ 500, 'D'], [ 400, 'CD'], [ 100, 'C'],
      [  90, 'XC'], [  50, 'L'], [  40, 'XL'], [  10, 'X'],
      [   9, 'IX'], [   5, 'V'], [   4, 'IV'], [   1, 'I'],
    ];

    let str = '';
    for (let i = 0; i < LETTERS.length; i++) {
      while (num >= LETTERS[i][0]) {
        str += LETTERS[i][1];
        num -= LETTERS[i][0];
      }
    }
    return str;
  }
}

////////////////////////////////////////////

var romanNumber1 = new RomanNumber('XX');
var romanNumber2 = new RomanNumber(40);
console.log(romanNumber1.toInt()); // => 20
console.log(romanNumber1.toString()); // => ‘XX’
console.log(romanNumber2.toInt()); // => 40
console.log(romanNumber2.toString()); // => ‘XL’