Converting from Roman numerals to Arabic numbers

Question

I got an idea to make a Roman/Arabic number converter, and I would like to hear your thoughts, where I could improve it, if there are Python specific things that I missed, so on and so forth.

# Decodes roman numerals
def get_arabic_numbers(t):
    # Hold the numbers in the list
    l = list()

    # The format
    format = 0

    # Convert from roman numeral to arabic number
    for i in range(0, len(t)):
        if t[i] == 'I':
            l.append(int(1))
        elif t[i] == 'V':
            l.append(int(5))
        elif t[i] == 'X':
            l.append(int(10))
        elif t[i] == 'L':
            l.append(int(50))
        elif t[i] == 'C':
            l.append(int(100))
        elif t[i] == 'D':
            l.append(int(500))
        elif t[i] == 'M':
            l.append(int(1000))

    # Calculate the format as follows:
    i = 0
    while i < len(l) - 1:
        # If the next numeral is greater than the current one, add the next one minus the current one = format = format + next_numeral - current_numeral
        if l[i] < l[i+1]:
            format = format + l[i+1] - l[i];
            i = i + 2
        # Else add it normally
        else:
            format = format + l[i]
            i = i + 1

    # Fix in case the last two numerals are equal
    if i == len(l):
        if l[i-2] == l[i-1]:
            format = format + l[i-1]
    else:
        format = format + l[i]

    # Return the format
    return format

---

# Encode an arabic digit to a roman numeral
def encode(l, c, one, five, nine):
    # The format to be returned
    format = ""

    # Special case 5.1: 5 has its own numeral 'V'
    # Special case 5.2: 50 has its own numeral 'A'
    # Special case 5.3: 500 has its own numeral 'D'
    if l[c] == 5:
        format = format + five
    elif l[c] < 5:
        for i in range(0, l[c]):
            format = format + one
    elif l[c] > 5:
    # Special case 9.1: 9 depends on 10, so it's 'IX'
    # Special case 9.2: 90 depends on 1000, so it's 'XC'
    # Special case 9.3: 900 depends on 1000, so it's 'CM'
        if l[c] == 9:
            format = format + nine
        else:
            format = format + five
            for i in range(6, l[c] + 1):
                format = format + one

    return format

---

# Encodes arabic numbers
def get_roman_numerals(t):
    # The arabic number to be converted
    t = int(t)
    # The list contains the number's digits
    l = list()

    # int is not iterable, so I wrote this to get the digits
    while t >= 1:
        l.append(int(t % 10))
        t = t / 10

    # They are in the wrong order, so the list has to be reversed
    l.reverse()

    # The format
    format = ""

    # Current position in the number
    c = 0

    # If the number is at least 1000
    if len(l) >= 4:
        num = 0

    # All the digits after 1000
    i = int(len(l) - 1)
    while i > 3:
        num = num + 1
        i = i - 1   

    # A list containing these digits
    li = list()

    for i in range(0, num + 1):
        li.append(l[i])

    # Converts the list to an int
    n = map(str, li)
    n = ''.join(n)
    n = int(n)    

    # Add an 'M' for every 1000
    for i in range(0, n):
        format = format + 'M'

    c = num + 1

    # If the number is also least 100
    if len(l) >= 3:
        format = format + encode(l, c, 'C', 'D', "CM")
        c = c + 1

    # If the number is at least 10
    if len(l) >= 3:
        format = format + encode(l, c, 'X', 'L', "XC")
        c = c + 1

    # If the number is at least 100 but smaller than 1000
    if len(l) > 2:
        format = format + encode(l, c, 'I', 'V', "IX")

    # If the number is at least 10 but smaller than 100
    if len(l) == 2:
        format = format + encode(l, c, 'X', 'L', "XC")
        c = c + 1
        format = format + encode(l, c, 'I', 'V', "IX")

    # If the number is at least 1 but smaller than 10   
    if len(l) == 1:
        format = format + encode(l, c, 'I', 'V', "IX")

    return format

Answer 1

Impressions

That's a very long solution. The comments were generally helpful, but the code itself could have been easier to understand if it were shorter and more expressive.

The variable names were generally not helpful: l, i, t, c, li were all cryptic.

get_arabic_numbers(t) is not really named appropriately:

• "get" implies retrieving something that already exists. This is more of a calculation.
• Why is "numbers" plural?
• Why is the parameter named t — what does it stand for?
• Strictly speaking, the result is an int. It's not Arabic, nor is it even in base ten. It's just an abstract integer, which is conventionally rendered for you as base-ten Arabic digits when str() is called on it (whether explicitly or implicitly). (And by "Arabic", we mean 0123456789, not ٠١٢٣٤٥٦٧٨٩.)

Based on these concerns, I'd call it decode_roman_numeral(roman). It happens to be what you wrote as the comment! Similarly, I'd call the inverse function encode_roman_numeral(num).

Arabic → Roman

# The format
format = 0

That's not what I would call a "format" — I would call it a "result".

---

# Hold the numbers in the list
l = list()
…

# Convert from roman numeral to arabic number
for i in range(0, len(t)):
    if t[i] == 'I':
        l.append(int(1))
    elif t[i] == 'V':
        l.append(int(5))
    elif t[i] == 'X':
        l.append(int(10))
    elif t[i] == 'L':
        l.append(int(50))
    elif t[i] == 'C':
        l.append(int(100))
    elif t[i] == 'D':
        l.append(int(500))
    elif t[i] == 'M':
        l.append(int(1000))

As I mentioned before, l is a cryptic name. There is no need to convert 1 to int(1). Lookups are typically done in Python using a dictionary. Also, whenever you populate a list by creating an empty list and appending to it repeatedly, that is a good candidate for a list comprehension:

trans = {'I': 1, 'V': 5, 'X': 10, 'L': 50, 'C': 100, 'D': 500, 'M': 1000}
values = [trans[r] for r in roman]

---

# Calculate the format as follows:
i = 0
while i < len(l) - 1:
    # If the next numeral is greater than the current one, add the next one minus the current one = format = format + next_numeral - current_numeral
    if l[i] < l[i+1]:
        format = format + l[i+1] - l[i];
        i = i + 2
    # Else add it normally
    else:
        format = format + l[i]
        i = i + 1

Do you really need a case that advances i by two? How about this instead:

result = 0
for i in range(0, len(l) - 1):
    if l[i] < l[i+1]:
        result -= l[i]
    else:
        result += l[i]

As I mentioned before, the code could be more expressive. This loop is summing values in a list, so we should use the sum() built-in function to say what we mean.

result = sum(
    val if val >= next_val else -val
    for val, next_val in zip(values[:-1], values[1:])
)

All together, then, I'd condense your function down to this:

def decode_roman_numeral(roman):
    """Calculate the numeric value of a Roman numeral (in capital letters)"""
    trans = {'I': 1, 'V': 5, 'X': 10, 'L': 50, 'C': 100, 'D': 500, 'M': 1000}
    values = [trans[r] for r in roman]
    return sum(
        val if val >= next_val else -val
        for val, next_val in zip(values[:-1], values[1:])
    ) + values[-1]

Arabic → Roman

The encode() function encodes a single Arabic digit, so encode_digit() would be a better name. There is no point in passing l and c separately, since you only ever care about l[c]. Instead of looping to append characters to format, use the * operator to construct a repeated string.

I would write this helper function using a single expression with four cases:

def encode_digit(digit, one, five, nine):
    return (
        nine                     if digit == 9 else
        five + one * (digit - 5) if digit >= 5 else
        one + five               if digit == 4 else
        one * digit              
    )

The get_roman_numerals() function is rather convoluted. I had a hard time following the code, so I went by your comments instead. In the end, I figured out that it was essentially doing this:

def encode_roman_numeral(num):
    num = int(num)
    return (
        'M' * (num // 1000) +
        encode_digit((num // 100) % 10, 'C', 'D', 'CM') +
        encode_digit((num //  10) % 10, 'X', 'L', 'XC') +
        encode_digit( num         % 10, 'I', 'V', 'IX') 
    )

Answer 2

I'll focus on get_arabic_numbers. First, a few local issues:

# Hold the numbers in the list
l = list()

You can come up with a better name than l which tells us nothing about what's in the list.

# The format
format = 0

You call it format which makes it look like it's a string, but it's actually a number. Also, "The format" adds no value as a comment. Either find something more descriptive that cannot be inferred by looking at the code or remove the comment.

# Convert from roman numeral to arabic number
for i in range(0, len(t)):

Simply use for c in t. It's very rare to use index-based loops in Python because most of the time you can simply iterate over the elements of your collection.

    if t[i] == 'I':
        l.append(int(1))

Why int(1)? 1 is enough.

    elif t[i] == 'V':
        l.append(int(5))

This is quite repetitive, and you can use a dictionary here. Simply declare it as a constant like this:

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

Now you can rewrite this whole loop like this:

for c in t:
    l.append(ROMAN_TO_INT[c])

You even get an exception if the letter is not in your dictionary, while your previous code silently ignored the issue (you had no else case). Note that you could transform this into a generator function, like this:

def convert_individual_letters(roman_string):
    for c in roman_string:
        yield ROMAN_TO_INT[c]

There are three benefits of doing this:

1. You don't need a name for l
2. It's a generator that can directly be used in a for loop (it can even more efficient if the rest of your code use generators too because you won't iterate over the whole string unless you asked for it - however it's not the case here)
3. You get to name your function, which improves clarity

And you can use this function in two ways. Either simply l = list(convert_individual_letters('MDC')) to consume the whole generator or with a for loop: for n in convert_individual_letters('MDC').

Okay, so now you converted something like MDCCCLXXIX to [1000, 500, 100, 100, 100, 50, 10, 10, 1, 10] and the goal is to get 1879.

# Calculate the format as follows:
i = 0
while i < len(l) - 1:

In this case, it looks like you do need an index, so a while loop is appropriate.

    # If the next numeral is greater than the current one, add the next one minus the current one = format = format + next_numeral - current_numeral
    if l[i] < l[i+1]:
        format = format + l[i+1] - l[i];
        i = i + 2
    # Else add it normally
    else:
        format = format + l[i]
        i = i + 1

Great, makes sense. Use format += l[i] instead of format = format + l[i]. One possible issue: If I send you "IC", you'll happily say 99. Is this valid?

# Fix in case the last two numerals are equal
if i == len(l):
    if l[i-2] == l[i-1]:
        format = format + l[i-1]
else:
    format = format + l[i]

Hmm, don't you think there's a better way than having to fix this case? One option is to add similar numbers together in convert_individual_letters, an another one is to change the loop:

i = 0
while i < len(l):
    # If the next numeral is greater than the current one, add the next one minus the current one = format = format + next_numeral - current_numeral
    if i < len(l) - 1 and l[i] < l[i+1]:
        format = format + l[i+1] - l[i];
        i = i + 2
    # Else add it normally
    else:
        format = format + l[i]
        i = i + 1

Finally, add test cases!