CODEWITHSUNDEEP
javascriptroman-numerals
Nilzone-
Nilzone-

Reputation: 2786

Roman to Int converts incorrectly in some cases

My function looks like this:

let romanToInt = romanNumber => {
    if(typeof romanNumber !== 'string') throw new TypeError('Argument must be of type String');

    const values = { 'I': 1, 'V': 5, 'X': 10, 'L': 50, 'C': 100, 'D': 500, 'M': 1000 };
    let sum = 0;

    romanNumber.split('').map(n => n.toUpperCase()).forEach(n => sum = (sum >= values[n]) ? sum + values[n] : values[n] - sum);

    return sum;
}

console.log(romanToInt("MCMXCVI"));

Most of the inputs I test with is correct, but the roman number MCMXCVI for example, should give me 1996, and not 2216, which is what I'm getting.

I found this, but I'm unsure how to implement that:

You must separate ones, tens, hundreds, and thousands as separate items. That means that 99 is XCIX, 90 + 9, but never should be written as IC. Similarly, 999 cannot be IM and 1999 cannot be MIM.

Upvotes: 2

Views: 105

Answers (3)

J S
J S

Reputation: 1198

I don't think you can implement with a one-liner mapping, because the operation depends on the comparisons of characters next to each other. However, with a simple for loop：

var chars = romanNumber.split('');
var sum = values[chars[chars.length - 1]];
for (var i = chars.length - 2; i >= 0; i--) {
    if (values[chars[i + 1]] <= values[chars[i]]) {
        sum += values[chars[i]];
    } else {
        sum -= values[chars[i]];
    }
}

The thing is when a roman digit is smaller in value than the one to its right, you want to subtract it from the sum instead of add.

Upvotes: 0

Matt Way
Matt Way

Reputation: 33161

As per your question, you need to cater to the separate items. An easy way to do this is to simply make your value set larger, and looking for multi char matches. This is possible because there are only a few combinations allowed with roman numerals. I have put a fiddle together here

const values = { 
  'I': 1, 
  'V': 5, 
  'X': 10, 
  'L': 50, 
  'C': 100, 
  'D': 500, 
  'M': 1000,
  'CM': 900,
  'CD': 400,
  'XC': 90,
  'XL': 40,
  'IX': 9,
  'IV': 4
};

let sum = 0;

while(romanNumber.length > 0){
  let piece = romanNumber.substring(0,2);
  if(values[piece]){
    sum += values[piece];
    romanNumber = romanNumber.substring(2);
  }else if(values[piece[0]]){
    sum += values[piece[0]];
    romanNumber = romanNumber.substring(1);
  }
}

return sum;

Upvotes: 1

Ruslan Batdalov
Ruslan Batdalov

Reputation: 793

You should not hope to solve the problem with a simple mapping, since in Roman numerals, the meaning of a digit depends on the context. I think, many cases can be handled with something like that (instead of your line with split and map):

var digits = romanNumber.split('');
var i = 0;
while (i < digits.length) {
    if (i == digits.length - 1 || values[digits[i]] >= values[digits[i+1]]) {
        sum += values[digits[i]];
        i++;
    }
    else {
        sum += values[digits[i+1]] - values[digits[i]];
        i += 2;
    }
}

But I am not sure that it will work in all cases. It would be better to see how it is implemented in ready libraries, maybe for other languages. For example, here is an implementation for Perl: http://search.cpan.org/~chorny/Roman-1.24/lib/Roman.pm

Upvotes: 0

Related Questions