2

I am trying to solve this python problem with python3, my code looks like this.

class Solution:
    def romanToInt(self, s: str) -> int:
        # Define integer value to each roman 
        rom_val = {'I': 1, 'V': 5, 'X': 10, 'L': 50,
                  'C': 100, 'D': 500, 'M': 1000}
        # A list of integer values
        value = list(map(rom_val.get, s))
        # The subtracted new number
        new = 0
        # The converted integer number
        integer = 0
        # List to keep the checked roman
        checked = []
        for i, j in enumerate(value):
            if j > value[i+1] or j == value[i+1]:
                checked.append(j)
            if j < value[i+1]:
                new = value[i+1] - j
                checked.append(new)
        return sum(checked)

However, I am getting IndexError: list index out of range on the first if statement. Even though I know this is rather an easy type of question but there is something I don't understand about. So I have two questions: 1. of course, why I am getting this index error? how do I fix it? 2. is my way of solving this problem correct?

Thank you very much.

Improve this question
5
  • With enumerating you'll iterate over all the indexes, and as you access i+1 , when you reach the final one, i+1 is out of the list Commented May 10, 2020 at 21:48
  • You probably want to replace IV by IIII , IX by VIIII etc for other "shorthands" before calculating the sum. Commented May 10, 2020 at 21:51
  • Post sample input and the full traceback. Commented May 10, 2020 at 22:03
  • There is a module for this. Commented May 10, 2020 at 22:53
  • Think carefully about what for i, j in enumerate(value): means. How many times should this loop run? What will be the value of i the last time through the loop? Therefore, should i+1 be expected to be a valid index into value? Commented Jan 10, 2023 at 5:57

5 Answers 5

Reset to default
5

Here is a different approach, 5 lines:

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

def romanToInt(self, s):
    res, p = 0, 'I'
    for c in s[::-1]:
        res, p = res - d[c] if d[c] < d[p] else res + d[c], c
    return res

Basically, going backward, adding each letter to result unless something smaller is in front of something larger in which case a subtraction instead of addition.

Note: The following is not a complete solution, just a fix for your error mentioned in the question. There is a major bug in your algorithm.

Like MCMXCIV should be 1994 but it returns 3099. This is because you consider C as 100 and M as 1000 but should be considering CM as 900. Since there is a solution above, I'll leave this as an exercise for you.

Problem with your code is that even if you reach the last index, you check for i + 1. You can fix that like this:

def romanToInt(s: str) -> int:
        # Define integer value to each roman 
        rom_val = {'I': 1, 'V': 5, 'X': 10, 'L': 50,
                  'C': 100, 'D': 500, 'M': 1000}
        # A list of integer values
        value = list(map(rom_val.get, s))

        # List to keep the checked roman
        checked = []
        for i, j in enumerate(value):
            if i == len(value) - 1:
                checked.append(j)
            elif j >= value[i+1]:
                checked.append(j)
            elif j < value[i+1]:
                checked.append(value[i+1] - j)

        print(checked)
        return sum(checked)

print(romanToInt("LVIII"))

I have also made your code a bit more concise, removed unnecessary variables. The major change is only the check if it is the last index on in value.

Improve this answer
Sign up to request clarification or add additional context in comments.

3 Comments

could you explain this "res, p = res - d[c] if d[c] < d[p] else res + d[c], c" a little bit
Going backwards, it handles the case that your algorithm completely misses. If something smaller is there in front of the current letter, you should be subtracting instead of adding.
Like MCMXCIV should be 1994 but it returns 3099. This is because you consider C as 100 and M as 1000 but should be considering CM as 900.
3

I know this is an answered post, but I would like to add 3 solutions with their respective runtimes to convert roman numerals to numbers.

Solution 1: (Approx Runtime = 52ms)

def romanToInt(self, s: str) -> int:

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

 for i in range(len(s)):

    if i!= len(s)-1 and roman[s[i]] < roman[s[i+1]]:
         num += roman[s[i]]*-1
    else:
         num += roman[s[i]]

  return num

Solution 2: (Approx Runtime = 60ms)

def romanToInt(self, s: str) -> int:

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

 s = s.replace("IV", "IIII").replace("IX", "VIIII")
 s = s.replace("XL", "XXXX").replace("XC", "LXXXX")
 s = s.replace("CD", "CCCC").replace("CM", "DCCCC")

 for x in s:
    num += roman[x]

 return num

Solution 3: (Approx Runtime = 48ms)

def romanToInt(self, s: str) -> int:

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

 for i in range(len(s)-1):
    if roman[s[i]] < roman[s[i+1]]:
        num += roman[s[i]]*-1
        continue

     num += roman[s[i]]

  num +=roman[s[-1]]

  return num

The simplest solution appears to be the best at times :)

Improve this answer

1 Comment

kudos for the answer, R.K.
0

This question can be solved with the idea that every roman numeral in the roman number are arranged in descending order if that's not the case then it is a special case like 'IV', 'IX', 'XL', .....

In the below the above peculiar case is handled by the if statement.

def romanToInt(s):
    mapping  = {
        'I': 1,
        'V': 5,
        'X': 10,
        'L': 50,
        'C': 100,
        'D': 500,
        'M': 1000,
        }
        
    min_ = None
    total = 0
    for c in s:
        val = mapping[c]
        print(val)
        if min_ and val > min_:
            total -= min_*2
        else:
            min_ = val
        total += val
                
    return total
Improve this answer

Comments

0

Take a look at this code. It's easy to understand the logic.

Note: The idea is to traverse the roman number in a reverse manner character by character and add if the next character is greater than or equal to the previous one.

def romanToDecimal(self, S): 
    d = {
        'I':1, 'V':5, 'X':10, 'L':50, 'C':100, 'D':500, 'M':1000
    }
    res = 0
    old = 'I'
    
    for i in S[::-1]:
        if d[i] >= d[old]:
            res = res + d[i]
        else:
            res = res - d[i]
        old = i
        
    return res
Improve this answer

Comments

-1

Here, for example, I pass 'II' to your function than values becomes [1,1]. Now using enumerate you will have (1,1) as i,j in your second iteration which is causing the index error. Then you can solve this easily.

Improve this answer

Comments