5
\$\begingroup\$

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);
};
\$\endgroup\$
  • \$\begingroup\$ Sorry for not posting an answer, I'm on the phone and won't be for a long time on my computer, so just trying to post a few notes. First, I don't see by their expectations that you need to check for faulty value or for a string numeral value. Second, you have some repetitive code that can be removed (after parseInt you basically do the same thing as you do with a numeral). Also your code is very nested and you bind private functions to this this making them public. I know it's just starters and not answering your question, that's why I'm only commenting :) \$\endgroup\$ – Chiptus Apr 9 '18 at 14:07
  • \$\begingroup\$ Thanks anyway for those elements of answer. About the checking for faulty values, they included examples of values I should test and some of those included string numeral. The only faulty value check I added on my own accord was the check for decimal values. However, I agree, I should have changed that 'else if' into a 'if' in an effort to make this function less redundant. Thanks also for the remark regarding the private functions. I hope you find the time to post an answer. \$\endgroup\$ – Ndna Apr 9 '18 at 17:13
6
\$\begingroup\$

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’

\$\endgroup\$
  • \$\begingroup\$ This is a great answer, thank you ! I understand what they meant by magic number. Thanks also for the remark on the validation regex. Your answer is extremely helpful. \$\endgroup\$ – Ndna Apr 10 '18 at 9:48

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.