HiveBrain v1.2.0
Get Started
← Back to all entries
snippetpythonMinor

Convert Roman to Int

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
convertintroman

Problem

I made a function that converts Roman letters to integers. But I'm not happy with it, as I feel like I used too much code for a such a simple task. Any suggestion on how I can minimize it?

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

def RomanToInt(year):
    result = 0
    boom = []
    for i in range(len(year)):
        for letter, value in values:
            if year[i] == letter:
                boom.append(value)
    boom.append(0)
    for i in range(len(year)):
        if boom[i] >= boom[i+1]:
            result = result + boom[i]
        else:
            result = result - boom[i]
    return result

Solution

A few notes:

-
A tuple of two-tuples is an odd structure for values, if you used a dictionary {'M': 1000, ...} you could avoid the loop over values and just use boom.append(values[year[i]]);

-
There is no point including e.g. CM in values, as there is no way year[i] (a single character) would ever be equal to those letters;

-
values should be defined inside the function, perhaps as a default argument, as it's not needed in any other scope;

-
for i in range(len(year)): ... year[i] is generally unpythonic, use for letter in year: instead (where you need i and i+1, you can use zip(l, l[1:]) to get pairs);

-
boom is not a very good name for the list, as it doesn't really explain what should be in it (something like numbers might be better) and there's no need to limit yourself to year inputs; and

-
Per the style guide, the function should be called roman_to_int, and you should consider adding a docstring.

With the above fixes:

def roman_to_int(roman, values={'M': 1000, 'D': 500, 'C': 100, 'L': 50, 
                                'X': 10, 'V': 5, 'I': 1}):
    """Convert from Roman numerals to an integer."""
    numbers = []
    for char in roman:
        numbers.append(values[char]) 
    total = 0
    for num1, num2 in zip(numbers, numbers[1:]):
        if num1 >= num2:
            total += num1
        else:
            total -= num1
    return total + num2


You could also consider simplifying with a list comprehension, and adding error handling for the following cases:

  • roman is an empty string ""; and



  • roman contains characters not in values.

Code Snippets

def roman_to_int(roman, values={'M': 1000, 'D': 500, 'C': 100, 'L': 50, 
                                'X': 10, 'V': 5, 'I': 1}):
    """Convert from Roman numerals to an integer."""
    numbers = []
    for char in roman:
        numbers.append(values[char]) 
    total = 0
    for num1, num2 in zip(numbers, numbers[1:]):
        if num1 >= num2:
            total += num1
        else:
            total -= num1
    return total + num2

Context

StackExchange Code Review Q#68297, answer score: 8

Revisions (0)

No revisions yet.