patternpythonMinor
A roman to integer converter
Viewed 0 times
romanconverterinteger
Problem
For an interview question I made a roman to integer converter:
As you can see I sometimes miss the last digit because I didn't want to get an index error. To avoid that I added an extra attribute that would check if the last element was checked or not (in cases where
Having an extra check and using extra space just for one element is highly erroneous. Where am I going wrong in my logic?
def romanToInt(self, s):
"""
:type s: str
:rtype: int
"""
mapping = {"I": 1, "V":5, "X":10, "L":50, "C":100, "D":500, "M":1000}
numeral_list = list(s)
num_list = [[mapping[i], -1] for i in numeral_list]
count = 0
i = 0
while(i < len(num_list) - 1):
if num_list[i] < num_list[i + 1]:
count += num_list[i + 1][0] - num_list[i][0]
num_list[i+1][1] = 1
num_list[i][1] = 1
i += 2
else:
count += num_list[i][0]
num_list[i][1] = 1
i += 1
if num_list[-1][1] == -1:
count += num_list[-1][0]
return countAs you can see I sometimes miss the last digit because I didn't want to get an index error. To avoid that I added an extra attribute that would check if the last element was checked or not (in cases where
s[len(s)-2] s[len(s)-1] then s[len(s)-1] is not checked. Having an extra check and using extra space just for one element is highly erroneous. Where am I going wrong in my logic?
Solution
You should stick to variable names according to the python style guide, PEP8. This means using
You could also use more descriptive names, for example
The complicated checking you do with having 2-tuples and deciding on that can be circumvented by iterating over
lower_case for variables and functions.You could also use more descriptive names, for example
roman instead of s.The complicated checking you do with having 2-tuples and deciding on that can be circumvented by iterating over
zip(numbers, numbers[1:]) to have both the number and the following number available at the same time. Still, we need to manually add the last digit at the end. Similar to the answer to this question I would propose to use something like:def roman_to_int(roman):
"""Convert from Roman numerals to an integer."""
values = {'M': 1000, 'D': 500, 'C': 100, 'L': 50,
'X': 10, 'V': 5, 'I': 1}
numbers = [values[char] for char in roman]
total = 0
for num1, num2 in zip(numbers, numbers[1:]):
if num1 >= num2:
total += num1
else:
total -= num1
return total + num2Code Snippets
def roman_to_int(roman):
"""Convert from Roman numerals to an integer."""
values = {'M': 1000, 'D': 500, 'C': 100, 'L': 50,
'X': 10, 'V': 5, 'I': 1}
numbers = [values[char] for char in roman]
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#137915, answer score: 4
Revisions (0)
No revisions yet.