snippetpythonMinor
Convert Roman to Int
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 resultSolution
A few notes:
-
A tuple of two-tuples is an odd structure for
-
There is no point including e.g.
-
-
-
-
Per the style guide, the function should be called
With the above fixes:
You could also consider simplifying with a list comprehension, and adding error handling for the following cases:
-
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 + num2You could also consider simplifying with a list comprehension, and adding error handling for the following cases:
romanis an empty string""; and
romancontains characters not invalues.
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 + num2Context
StackExchange Code Review Q#68297, answer score: 8
Revisions (0)
No revisions yet.