Converting roman numerals to integers

Question

I've been given a task to convert roman numerals to integers and have been able to come up with the following solution:

def roman_numeral_to_int(string):
    symbols = {
        'I': 1,
        'V': 5,
        'X': 10,
        'L': 50,
        'C': 100,
        'D': 500,
        'M': 1000
    }
    repetitions = {}
    result = 0
    skip = 0
    for i, c in enumerate(string):
        if i == skip and i != 0:
            continue
        if c not in symbols:
            return None
        if c in repetitions.items():
            repetitions[c] += 1
        else:
            repetitions = {c: 1}
        for r, v in repetitions.items():
            if (r in ['L', 'D', 'V'] and v > 1) or (r in ['I', 'X', 'C'] and v > 3):
                return None
        if c == 'I':
            # last character in the string
            if i == len(string) - 1:
                result += 1
            elif string[i+1] == 'V':
                result += 4
                skip = i + 1
            elif string[i+1] == 'X':
                result += 9
                skip = i + 1
        elif c == 'X':
            # last character in the string
            if i == len(string) - 1:
                result += 10
            elif string[i+1] == 'L':
                result += 40
                skip = i + 1
            elif string[i+1] == 'C':
                result += 90
                skip = i + 1
        elif c == 'C':
            # last character in the string
            if i == len(string) - 1:
                result += 100
            elif string[i+1] == 'D':
                result += 400
                skip = i + 1
            elif string[i+1] == 'M':
                result += 900
                skip = i + 1
        else:
            skip = 0
            result += symbols[c]
    return result

However, this solution gets the wrong answer with the string MLXVI which should output 1066, while this code yields 1056.

Can someone please point out what's wrong with this solution?

Possible duplicate of [Converting Roman Numerals to integers in python](https://stackoverflow.com/questions/19308177/converting-roman-numerals-to-integers-in-python) — cdarke, Feb 15 '18 at 07:25
@cdarke while the question might not be exactly new, I would like to know what's wrong with my approach. — Mahmoud Hanafy, Feb 15 '18 at 07:36
@ReblochonMasque Yes, I know it's skipping the X, and I'd like to know why. — Mahmoud Hanafy, Feb 15 '18 at 07:36

score 1 · Answer 1 · answered Feb 15 '18 at 07:37

Just quickly reading, but...

if i == skip and i != 0 wouldn't need a special case for the first characted if skip wasn't precisely 0.

if c in repetitions.items() seems exceedingly unlikely to work; the items are tuples and c is a character.

repetitions should probably be a collections.Counter.

You've written special cases for a bunch of characters (IXC) even though the rule is consistent: lower magnitude before higher magnitude is subtracted. Among other things, these special cases have hardcoded values.

Those special cases include an implicit case where characters are ignored.

score 0 · Accepted Answer · answered Feb 15 '18 at 07:35

That specific case would be fixed by adding an else statement in this block:

elif c == 'X':
    # last character in the string
    if i == len(string) - 1:
        result += 10
    elif string[i+1] == 'L':
        result += 40
        skip = i + 1
    elif string[i+1] == 'C':
        result += 90
        skip = i + 1
    else:
        result += 10

However I see that this may be happening in other places like with multiple Is. So III should be 3 but your program returns 1.

Thank you, I didn't know that. – Mahmoud Hanafy Feb 15 '18 at 07:37 — Mahmoud Hanafy, Feb 15 '18 at 07:37

Converting roman numerals to integers

2 Answers2