Jeremy Fisher Jeremy Fisher - 4 months ago 8
Python Question

Any way to remove the last check in my converter?

For an interview question I made a roman to integer converter:

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 count


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 s[len(s)-2] < s[len(s)-1], s[len(s)-1] is checked, but if 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?

EDIT: Here was my code before

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] 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] - num_list[i]
i += 2
else:
count += num_list[i]
i += 1
return count


It failed on several test cases since it did not count the last digit

Answer

What they are looking for before anything else is whether your code is easily readable and maintainable. That's more important than being right, in my opinion, although the interviewer may disagree -- so make it also correct.

Check for invalid inputs. What if they give a string like 'VXQIII'? Do you want to enforce any rules or are you okay with giving 'VVVVV' = 25?

Throw in a unit test or test function for bonus points.

You invent a complicated structure with mysterious code numbers instead of using a clearly named variable like 'has_been_counted'. This makes your code hard to read. Therefore all your code will be hard for other programmers to read and maintain. And when I say other programmers, I mean you, next week, when you can't remember why you did that.

Additionally, that seen flag is unnecessary. You already have an array index telling you what you have and have not seen.

Python specific:

For an interview use pep-8. You can figure out how strict they are about style later, but python people can be pickier about that than most languages.

Self is unused, and it's not shown as being a class member anyway. "print romanToInt('XCIV')" will raise an error.

Speaking of errors, Python people may appreciate catching invalid characters with a try..except around the mapping lookup, and then reraise as ValueError('Not a valid roman numeral'). Maybe a python person can comment on what exception type to use for that, but I think ValueError is the right one.

converting s to a list of characters is unnecessary. You can already iterate a string as a list of characters.

for letter in 'XCIV':
    print letter

X
C
I
V
Comments