Roman to Integer in JS why it only convert the first character

Question

I try to solve the Leedcode question 13 and the question is Given a roman numeral, convert it to an integer.(Input is guaranteed to be within the range from 1 to 3999.) This is my code below, I wonder why it only converts the first character in the roman numeral to integer?

var romanToInt = function(s) {
  var result = 0;
  if (s == null) {
    result = 0;
  }
  var myMap = new Map();
  myMap.set('I', 1);
  myMap.set('V', 5);
  myMap.set('X', 10);
  myMap.set('L', 50);
  myMap.set('C', 100);
  myMap.set('D', 500);
  myMap.set('M', 1000);

  var len = s.length;
  for (var i = 0; i < len; i++) {
    if (myMap.get(s.charAt(i)) < myMap.get(s.charAt(i + 1))) {
      result -= myMap.get(s.charAt(i))
    } else {
      result += myMap.get(s.charAt(i))
    }
    return result;
  };
}

console.log(romanToInt('VI'));
console.log(romanToInt('V'));
console.log(romanToInt('VII'));

Fechner

As an aside, each function call creates the entire Map all over again. You should move it outside your function. – Footnote

@Fechner Your logic is perfectly alright,put "return" statement after the loop since you're calculating and returning same time. – Finedraw

Answer 1

Because

return result;

ends the loop after one iteration. Move it down one line. And if you would do proper formatting, you wouldn't make such mistakes at all ;)

const values = new Map([
  ['I', 1],
  ['V', 5],
  ['X', 10]
  /*....*/
]);

function romanToInt(string) {
  let result = 0,
    current, previous = 0;
  for (const char of string.split("").reverse()) {
    current = values.get(char);
    if (current >= previous) {
      result += current;
    } else {
      result -= current;
    }
    previous = current;
  }
  return result;
}

console.log(romanToInt('I'));
console.log(romanToInt('II'));
console.log(romanToInt('III'));
console.log(romanToInt('IV'));
console.log(romanToInt('V'));
console.log(romanToInt('VI'));
console.log(romanToInt('VII'));
console.log(romanToInt('VIII'));
console.log(romanToInt('IX'));
console.log(romanToInt('X'));
console.log(romanToInt('XI'));
console.log(romanToInt('XII'));
console.log(romanToInt('XIII'));
console.log(romanToInt('XIV'));
console.log(romanToInt('XV'));
console.log(romanToInt('XVI'));
console.log(romanToInt('XVII'));
console.log(romanToInt('XVIII'));
console.log(romanToInt('XIX'));
console.log(romanToInt('XX'));

Milla

Haha that's a nice catch :) – Halmstad

@Fechner Don't forget to mark an answer as such if your problem is solved 😉 – Leban

string.reverse()??? And even if we fix that, the results will not be correct. Have you tried it before posting? – Rogan

THANKS FOR REMINDING ME THAT!@Leban – Fechner

You fixed the string.reverse(), but the logic is still incorrect and will give wrong results :(. – Rogan

@racil could you give an example where it goes wrong? – Milla

Like I said, you should test your code, it's not that hard. I converted your code to a snippet and added test examples from 1 to 20. Run it and see the results. I don't think they need explanation :). On the other hand, if you try the code from OP (after fixing the return) or from the other answer, they both produce the correct results from 1 to 20. You didn't need to propose a solution since the OP's solution works, so you can either remove the solution from your answer, or fix it. – Rogan

@racil oh come on. That counts as a typo :) – Milla

It sure does, but nevertheless needed fixing :) – Rogan

Answer 2

Because your return result; is inside loop, just move return result; to outside loop.

below is simplified(readability wise) version of your code.

const myMap=new Map();
myMap.set('I', 1);
myMap.set('V', 5);
myMap.set('X', 10);
myMap.set('L', 50);
myMap.set('C', 100);
myMap.set('D', 500);
myMap.set('M', 1000);

var romanToInt = function(s) {
   var result=0;
   if(s){
     var s1=s.split('');
     s1.forEach(function(e,i){
          result += myMap.get(e) < myMap.get(s1[i+1]) ? -myMap.get(e) : myMap.get(e);  // used ternary oprator with '-' where required
     });
   }
   return result; //move it outside loop
}
console.log(romanToInt('IV'));
console.log(romanToInt('V'));
console.log(romanToInt('VII'));

Pizzeria

Answer 3

At first I declare the romanVal as an object containing all of the roman symbols' values and set the result to zero at step two, then if (s === '') return; to check if user input was empty then it will return result = 0, else I use input and change it into an array and make a map on to it, each value in this array changed to a number and pushed to inputval array, then make a loop on this array to check if element[i] < or > element[i + 1] to add it or subtract it from the result.

var romanToInt = function (s) {
    let romanVal = { I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000 };
    let result = 0;
    if (s === '') return;
    let inputs = Array.of(...s);
    let inputsVal = [];
    inputs.map((e) => ((e = romanVal[e]), inputsVal.push(e)));
    for (let i = 0; i < inputsVal.length; i++) {
        inputsVal[i] < inputsVal[i + 1]
            ? (result -= inputsVal[i])
            : (result += inputsVal[i]);
    }
    return result;
};

romanToInt('III')

Schnitzel

Could do with an explanation – Polson

Yeah sure, at first I declared the romanVal as an object containing all of the roman symbols' values and set the result to zero at step two, then ` if (s === '') return;` to check if user input was empty then it will return result = 0, else I use input and change it into an array and make a map on to it, each value in this array changed to a number and pushed to inputval array, then make a loop on this array to check if element[i] < or > element[i + 1] to add it or subtract it from the result, I wish this comment clarifies it for you. – Schnitzel

That's a good explanation! You can see I added it into your answer, which is now a high quality answer for the site. – Polson

I don't think you should use a map function, when you don't actually use the map. I would change this to use a forEach loop. inputs.forEach((e) => ((e = romanVal[e]), inputsVal.push(e))); – Breeze

Answer 4

A shorter version of Roman to Integer:

var romanToInt = (str) => {
  const roman = { I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000 };
  let num = 0;
  for (let i = 0; i < str.length; i++) {
    const curr = roman[str[i]];
    const next = roman[str[i + 1]];
    (curr < next) ? (num -= curr) : (num += curr);
  }
  return num;
};

console.log(romanToInt('IV'));
console.log(romanToInt('VIII'));
console.log(romanToInt('LXXIX'));
console.log(romanToInt('MCMXLIV'));

---

Another unique solution from @Jin150Job

var romanToInt = (str) => {
  const roman = { I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000 };
  let num = 0;
  if (str.includes('CM')) num -= 200;
  if (str.includes('CD')) num -= 200;
  if (str.includes('XC')) num -= 20;
  if (str.includes('XL')) num -= 20;
  if (str.includes('IX')) num -= 2;
  if (str.includes('IV')) num -= 2;
  for (var i = 0; i < str.length; i++) {
    num += roman[str[i]];
  }
  return num;
};

console.log(romanToInt('IV'));
console.log(romanToInt('VIII'));
console.log(romanToInt('LXXIX'));
console.log(romanToInt('MCMXLIV'));

Asthenosphere

Answer 5

A nice and preferable answer to this solution would be:

  let roman = "XXI";
  const numeral = {I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000};
  let total = 0;
  let current, last = 0;
  roman.split("").reverse().forEach(e => {
    current = numeral[e];
    if (current >= last) {total += current;} else {total -= current;}
    last = current;
  });
  
  return total;

Cardamom

Answer 6

Here is another solution to find the Integer number of a roman number

function romanToInt(romanNumber) {
   let currentValue = 0;
    let result = 0;
    const string = romanNumber.split('');
    const romanNumbers = {
      I: 1,
      V: 5,
      X: 10,
      L: 50,
      C: 100,
      D: 500,
      M: 1000,
    };
    let firstNum = romanNumbers[string[0].toUpperCase()];
    for (let char of string) {
      currentValue = romanNumbers[char.toUpperCase()];
      if (currentValue <= firstNum) {
        result += currentValue;
      } else {
        result = currentValue - result;
      }
    }
    console.log(`Roman: ${romanNumber}\nInteger: ${result} \n \n`);
    return result;
}

romanToInt('I');
romanToInt('IV');
romanToInt('VI');
romanToInt('IX');
romanToInt('X');
romanToInt('XI');
romanToInt('XII');
romanToInt('XL');
romanToInt('LV');
romanToInt('MMM');
romanToInt('MMC');
romanToInt('MMc');

Piscine

Answer 7

   const romanIndex = { I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000 };
   let number = 0;
   for (i = 0; i < s.length; i++) {
     const curr = romanIndex[s[i]];
     const next = romanIndex[s[i + 1]];
     if (curr < next) {
       number = number - curr;
     } else {
       number = number + curr;
     }
   }
   return number;
 };

 const romand = romanToInt("MCMXCIV");
 console.log(romand);```

Felipe

Answer 8

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

    let result = 0;

    for (let i = 0; i < s.length; i++) {
        const current = romanValues[s[i]];
        const next = romanValues[s[i + 1]];

        if (current < next) {
            result -= current;
        } else {
            result += current;
        }
    }

    return result;
}

// Example usage:
console.log(romanToInt("III"));       // Output: 3
console.log(romanToInt("LVIII"));     // Output: 58
console.log(romanToInt("MCMXCIV"));   // Output: 1994

    

Houseroom

Verified the implementation by testing with various Roman numerals, and it works correctly for all cases. – Houseroom

Answer 9

Maybe you could try :

function romanToInt(romanString) {
   if(typeof romanString !== 'string') return NaN;
   const romanRegExp = /^(?=[MDCLXVI])(M*)(?:(C)(?:(D)|(M))|(D?)(C{0,3}))(?:(X)(?:(L)|(C))|(L?)(X{0,3}))(?:(I)(?:(V)|(X))|(V?)(I{0,3}))$/i;
   let parts = romanString.match(romanRegExp);
   if(!parts) return NaN;
   let result = 0;
   [0, 1000, -100,500,1000,500,100, -10,50,100,50,10, -1,5,10,5,1].forEach(
      (weight, i) => result += weight * (parts[i] || '').length
   );
   return result;
}

console.log(romanToInt('IV'));
console.log(romanToInt('MMXIX'));

Cholula

Answer 10

Try this instead. It should work:

function conversion(r) {
  const arr = r.split('');
  const val = {
    I: 1,
    V: 5,
    X: 10,
    L: 50,
    C: 100,
    D: 500,
    M: 1000,
  };

  let sum = 0;
  for (let i = 0; i < arr.length; i += 1) {
    if (val[arr[i]] < val[arr[i + 1]]) {
      sum += val[arr[i + 1]] - val[arr[i]];
      i += 1;
    } else {
      sum += val[arr[i]];
    }
  } return sum;
}
      
console.log(conversion("VIII"));
console.log(conversion("LXXIX"));
console.log(conversion("MCMXLIV"));

Pricecutting

Answer 11

** this solution works for every roman number < 3600

function romanToInt(romeNumStr) {
    const romeMap = {I: 1, V: 5, X: 10, L: 50, C: 100, D: 500, M: 1000};
    let total = 0;
    for (let i=0; i<romeNumStr.length; i++) {
        const chVal = romeMap[romeNumStr[i]];
        let chNextVal
        if (i+1 < romeNumStr.length) {
            // There is more chars
            chNextVal = romeMap[romeNumStr[i+1]];
            if (chVal >= chNextVal) {
                total += chVal;
            } else {
                // Do reverse
                total += chNextVal-chVal
                i+=1
            }
        } else {
            // Last char
            total += chVal;
        }
    }
    return total
}

my test:

// TEST
romanToInt('MMMDXCIX')
=3599

Fructidor

Answer 12

var romanToInt = function(s) {
    const map = new Map();
    map.set("I", 1);
    map.set("V", 5);
    map.set("X", 10);
    map.set("L", 50);
    map.set("C", 100);
    map.set("D", 500);
    map.set("M", 1000);
    map.set("IV", 4);
    map.set("IX", 9);
    map.set("XL", 40);
    map.set("XC", 90);
    map.set("CD", 400);
    map.set("CM", 900);
    if(s.length == 1){
        return map.get(s);
    }
    let sum = 0;
    for(let i = 0; i <s.length;){
        const dub = map.get(s[i]+s[i+1]);
        if(dub == undefined){
          sum += map.get(s[i]);
          i++;
        }else{
          sum += dub;
          i+=2;
        }
        
    }
    return sum;

};

Broca

Answer 13

function getNumber(roman) {
   let romanValues= { I : 1,  V : 5, X : 10, L : 50, C : 100,  D : 500, M :1000 };
   function getSubtractionStatus(currRoman, nextRoman) {
      if(currRoman === nextRoman) return false;
      if(romanValues[currRoman] < romanValues[nextRoman]) return true; 
      return false;
   }
   let romanArr = roman.split('');
   let sum = 0;
   for(let i=0; i < romanArr.length; i++){
       if(getSubtractionStatus(romanArr[i],romanArr[i+1]) && i < romanArr.length - 1) {
          sum = sum + (romanValues[romanArr[i+1]] - romanValues[romanArr[i]]);
          i++;
       } else {
          sum = sum + romanValues[romanArr[i]]; 
       }
   }
   return sum;
}

console.log(getNumber('XIX'));

Kissel

Answer 14

Roman to Integer (asked in many interviews and its solution) . The solution is based on string methods.

var romanToInt = function (s) {
      let roman = { "I": 1, "V": 5, "X": 10, "L": 50, "C": 100, "D": 500, "M": 1000 }, sum = 0;
      if (roman[s]) {
        return roman[s];
      }
      else {
        for (let i = s.length - 1; i >= 0; i--) {
          if (s.charAt(i - 1) && roman[s.charAt(i - 1)] < roman[s.charAt(i)]) {
            sum += roman[s.charAt(i)] - roman[s.charAt(i - 1)];
            i--;
          } else {
            sum += roman[s.charAt(i)]
          }
        }
        return sum;
      }
    };
    romanToInt('IV');

Dmso

Answer 15

This worked for me

var romanToInt = function(s) {
    const main = {
        'I':1,
        'V':5,
        'X':10,
        'L':50,
        'C':100,
        'D':500,
        'M':1000,
    }
    var retrunValue = 0;
    for(let i = 0 ; i < s.length; i++){
        const currentValue = main[s[i]];
        const nextValue = main[s[i+1]];

        !!!nextValue ? retrunValue += currentValue :
        currentValue === nextValue ? retrunValue += currentValue :
        currentValue > nextValue ? retrunValue += currentValue : retrunValue -= currentValue
    }
    return retrunValue
};

//1994
console.log(romanToInt('MCMXCIV'))

Kathrinekathryn

Answer 16

Here's one using reduce

const romanNumerals = {
  I: 1,
  V: 5,
  X: 10,
  L: 50,
  C: 100,
  D: 500,
  M: 1000,
}

const convertToNumber = romanNumeral => {
  const romanDigits = romanNumeral.split('')

  return romanDigits.reduce(
    (acc, curr, index) =>
      romanNumerals[curr] < romanNumerals[romanDigits[index + 1]]
        ? acc - romanNumerals[curr]
        : acc + romanNumerals[curr],
    0
  )
}

console.log(convertToNumber('XXXIX'))
console.log(convertToNumber('XL'))
console.log(convertToNumber('MMXXI'))
console.log(convertToNumber('CLX'))
console.log(convertToNumber('DCCLXXXIX'))
console.log(convertToNumber('MCMXVIII'))
console.log(convertToNumber('MMMCMXCIX'))

Figone