Roman to Integer - JavaScript

Question

I want to convert the Roman Integers to Arabian. Here is my code:

/**
 * @param {string} s
 * @return {number}
 */
var romanToInt = function(s) {
    let romanInts = {
        I: 1,
        V: 5,
        X: 10,
        L: 50,
        C: 100,
        D: 500,
        M: 1000
    }

    let romanArr = s.split("").reverse();
    let res = 0;

      for(i = 0; i < romanArr.length; i++) {
          if(romanArr[i] < romanArr[i - 1]) {
             res -= romanInts[romanArr[i]];
          } else {
              res += romanInts[romanArr[i]];
          }
          console.log(romanArr)
         console.log(res);
      }

    return res
};

So this code works very simple. It starts iterating the Roman letters from the end comparing with the previous one and if current letter smaller than the previous it substracting it form the sum. For example, in "IV" letter "I" smaller than "V" and because "I" = 1 and "V" = 5 code substracting 5 from 1 and we get "5 - 1 = 4". But something went wrong...

For example in Case "LVIII" he gets like "I" + "I" + "I" + "V" = 8, but when it gets to the "L", code decide that "L" is smaller than "V" and insted of get "8 + 50 = 58" code substracted it form the sum so we get "8 - 50 = -42". And that not the single case.

In "MCMXCIV" comparing the "C" and "I" he decides that "I" is greater than "C" and we get "4 - 100 = -96".

I would so grateful if you can help me. Sorry if problen migth be stupid but I trying to solve it around few hours and still can't figure out where the problem is.

there are some console logs from the tests:

1. "LVII" [ 'I', 'I', 'I', 'V', 'L' ] 1 [ 'I', 'I', 'I', 'V', 'L' ] 2 [ 'I', 'I', 'I', 'V', 'L' ] 3 [ 'I', 'I', 'I', 'V', 'L' ] 8 [ 'I', 'I', 'I', 'V', 'L' ] -42

2. "MCMXCIV" [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] 5 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] 4 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] -96 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] -86 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] -1086 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] -1186 [ 'V', 'I', 'C', 'X', 'M', 'C', 'M' ] -186

Answer 1

I see this issue in your code:

You are comapring letters' encoding values inside the for loop instead of the numbers those letters represent.

so

 if(romanArr[i] < romanArr[i - 1]) {

is wrong because

console.log("L" < "V");

gives true when it should give false. It gives true because (in layman's terms) L comes before V in the alphabet and therefore the character encoding value of L is smaller than V.

Instead it should be

if(romanInts[romanArr[i]] < romanInts[romanArr[i - 1]]) {

since you want to compare the roman-numerical values of these letters and not the letters' encoding values

Here is a working version of your original code:

const romanToInt = function(s) {
    let romanInts = {
        I: 1,
        V: 5,
        X: 10,
        L: 50,
        C: 100,
        D: 500,
        M: 1000
    };

    let romanArr = s.split("").reverse();
    let res = 0;

      for(i = 0; i < romanArr.length; i++) {
          if(romanInts[romanArr[i]] < romanInts[romanArr[i - 1]]) {
             res -= romanInts[romanArr[i]];
          } else {
              res += romanInts[romanArr[i]];
          }
      }

    return res;
};

console.log(romanToInt("LVIII")); // gives 58
console.log(romanToInt("MMXXIII")); // gives 2023
console.log(romanToInt("DCCCXLV")); // gives 845

This works because the overall idea behind your code was correct and the logic was sound.

That said, it is usually good practice to avoid reversing arrays where possible. In this case it doesn't matter too much as the array won't be too large but if you wish to avoid reversing it, you could instead loop through the array backwards (so starting at i = arr.length-1 instead of i=0), and checking not the element before but the element after the current one, like so:

if(romanInts[romanArr[i]] < romanInts[romanArr[i + 1]]) // notice +1 and not -1

Here is an example:

const romanToInt = function(s) {
    let romanInts = {
        I: 1,
        V: 5,
        X: 10,
        L: 50,
        C: 100,
        D: 500,
        M: 1000
    };

    let romanArr = s.split("");
    let res = 0;

      for(i = romanArr.length-1; i >= 0; i--) {
          if(romanInts[romanArr[i]] < romanInts[romanArr[i + 1]]) {
             res -= romanInts[romanArr[i]];
          } else {
              res += romanInts[romanArr[i]];
          }
      }

    return res;
};

console.log(romanToInt("LVIII")); // gives 58
console.log(romanToInt("MMXXIII")); // gives 2023
console.log(romanToInt("DCCCXLV")); // gives 845