6
\$\begingroup\$

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
\$\endgroup\$
  • \$\begingroup\$ You may want to check out Mark Pilgrim's code: docutils.sourceforge.net/docutils/utils/roman.py \$\endgroup\$ – Alan Oct 29 '14 at 17:39
  • \$\begingroup\$ I see Mark is using variable[index:index] to match each value. Any way you can give me a reference or documentation for that? I've never heard of something like variable[0:2] \$\endgroup\$ – Tony Fire Oct 29 '14 at 17:58
  • 2
    \$\begingroup\$ @TonyFire look up "slice" \$\endgroup\$ – jonrsharpe Oct 29 '14 at 17:59
8
\$\begingroup\$

A few notes:

  1. 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]]);

  2. 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;

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

  4. 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);

  5. 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

  6. 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.
\$\endgroup\$
  • \$\begingroup\$ The problem I had with dictionary is that the values are unordered. I have a int_to_roman function also that checks the intergers in order of values \$\endgroup\$ – Tony Fire Oct 30 '14 at 14:48
  • \$\begingroup\$ Does the solution work for one-letter roman numbers? \$\endgroup\$ – Heikki Feb 6 '19 at 11:22
  • \$\begingroup\$ @Heikki apparently not, when I try it throws an error. Perhaps you can work out why? \$\endgroup\$ – jonrsharpe Feb 6 '19 at 11:26
  • \$\begingroup\$ zip(numbers, numbers[1:]) refers to an empty number and then num2 is never assigned. Adding num2 = numbers[0] in front of for ... zip(...) helped. \$\endgroup\$ – Heikki Feb 6 '19 at 11:52
  • 1
    \$\begingroup\$ @laza reading back through the comments that has been discussed before. The point here was not to provide a perfect implementation, I'll leave it as-is. \$\endgroup\$ – jonrsharpe Mar 18 at 18:05
0
\$\begingroup\$

This is fairly minimal.

def get_int_from_roman_numeral_string(r):
    x = {'M': 1000, 'D': 500, 'C': 100, 'L': 50, 'X': 10, 'V': 5, 'I': 1}
    return sum([x[i] if x[i] >= x[j] else -x[i] for i, j in zip(r, r[1:])]) + x[j]
  • As jonrsharpe has pointed out we can minimize your tuple-of-tuples (values) by removing the roman numeral strings of length two. Not knowing much at all about Roman numerals, I was curious how this worked. After reading the Roman numerals Wikipedia article it became clear that there are a couple of ways to evaluate a string of Roman numeral symbols. The type of Roman numeral strings that your code can process must utilize subtractive notation. A familiar example is IV == 4. The logic for subtractive notation is embedded in the conditional expression found in your final for loop.
  • Of course I wouldn't recommend usually writing code like the final line of my function. But as you were curious how to reduce your code length, I obliged my inner code golfer.
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.