Roman to Integer

Question

I'm trying to get better as a programmer and would love any feedback on my code. Especially the section in the while loop. How could I make this more readable? What would your opinion as an interviewer be of my code? I utilized python 2.7.

Challenge:

Given a Roman Numeral, convert it to an integer.

Example:

Input: "LVIII" Output: 58

Here is my Code:

def romanToInt(self, roman):
    if not (roman):
        return 0

    LetterToValue = {'I' : 1, 
                 'V' : 5,
                 'X' : 10,
                 'L' : 50,
                 'C' : 100,
                 'D' : 500,
                 'M' : 1000}

    RomanCharacterized = list(roman)
    End = len(RomanCharacterized)
    if (End == 1):
        return LetterToValue[RomanCharacterized[0]]

    start = 0
    next = start + 1
    result = 0


    while(next < End): 
                           #Value @ Start        <         Value @ Immediately Following 
        if(LetterToValue[RomanCharacterized[start]] < LetterToValue[RomanCharacterized[next]]):
            result = result + (LetterToValue[RomanCharacterized[next]] - LetterToValue[RomanCharacterized[start]])
                          #Value @ Start        ==         Value @ Immediately Following 
        if(LetterToValue[RomanCharacterized[start]] == LetterToValue[RomanCharacterized[next]]):
            result = result + (LetterToValue[RomanCharacterized[start]] + LetterToValue[RomanCharacterized[next]])
                          #Value @ Start        >         Value @ Immediately Following 
        if(LetterToValue[RomanCharacterized[start]] > LetterToValue[RomanCharacterized[next]]):
            result = result + LetterToValue[RomanCharacterized[start]]
            start = next #Needs to advance one slot
            next = next + 1
        else:
            start = next + 1 #advances two slots
            next = start + 1


    if (start == (End -1)):
        result = result + LetterToValue[RomanCharacterized[start]]

    return result

Answer 1

Packaging and style

Why does this function accept self as the first parameter? You never use it anywhere, and it doesn't look like this function should be a method in a class.

Stick to PEP 8 naming conventions (roman_to_int(), LETTER_TO_VALUE, end, etc.)

Avoid superfluous punctuation. None of these expressions should be written with parentheses: not (roman), if (End == 1), while(next < End).

A docstring with doctests would be a good idea.

Algorithm

I see a lot of minor unnecessary complications that together make the solution more cumbersome than necessary.

There is no need to convert roman into the list RomanCharacterized: you can index directly into strings, and you can call len() on strings.

There is no need for the if (End == 1) special case: the general case for the algorithm handles one-character inputs just fine.

I see LetterToValue[RomanCharacterized[…]] written everywhere! Why not convert the letters to numbers early on to avoid that verbosity? (See values in my solution below.)

start is not a good variable name, since it is incremented in the loop (in contrast to End, which stays fixed). Note that next is always start + 1, so there is an opportunity to either write assert next == start + 1 or to eliminate one of the variables.

Going further, if you are always considering neighbouring elements of a sequence, it may be better to use zip() in some way. Also, the loop logic would be much simpler if you never advance by two positions. As long as the input is legal (i.e. not like IIX), you can get the same results using the function below, which I consider to be more Pythonically expressive.

def roman_to_int(roman):
    """
    Given a valid Roman numeral, convert it to an integer.  (Results
    for invalid inputs are undefined; no validation is performed.)

    >>> roman_to_int('LVIII')
    58
    >>> roman_to_int('C')
    100
    >>> roman_to_int('')
    0
    """
    values = [
        {'I': 1, 'V': 5, 'X': 10, 'L': 50, 'C': 100, 'D': 500, 'M': 1000}[c]
        for c in roman
    ]
    return sum(
        +n if n >= next else -n
        for n, next in zip(values, values[1:] + [0])
    )

Answer 2

self argument

The self argument is only required if you are writing a method belonging to a class. In this case, it's unnecessary (and unused) so you can safely delete it.

---

In Python, the recommended method for casing of names is snake_case, with the exception of class names, which uses CamelCase. So, romanToInt instead of roman_to_int, LetterToValue would be letter_to_value, and so on.

---

As hoffmale mentioned in a comment, your code accepts invalid Roman numbers, such as LL, DD, and IIV:

print roman_to_int('LL') # 100
print roman_to_int('DD') # 1000
print roman_to_int('IIV') # 7

Before transforming the symbols to numbers, I would recommend to check to make sure that it is a valid Roman numeral, and perhaps raise ValueError('Invalid Roman numeral') if it is not, or something along those lines.