Adding two roman-numeral inputs

Question

This is the first coding class I've taken and I'm not getting much feedback from the instructor. The function of this program is to take two roman numeral inputs, convert them to arabic, add them together, then convert the sum to roman numerals. I wanted to see if any of you code ninjas could give me some feedback on the solution that I've come up with for this.

I would like to note that in the class I'm taking we haven't learned about dictionaries and we have only briefly covered lists.

This is only the functional part of the code, not including the loops to catch improper inputs.

print('Enter two roman numeral number to be added together')
print()

#converting first roman numeral to arabic

user1 = input('Enter first roman numeral: ')

print()

total1 = 0

for i in user1:
    if i == 'I':
        total1 += 1 
    elif i == 'V':
        total1 += 5
    elif i == 'X':
        total1 += 10
    elif i == 'L':
        total1 += 50
    elif i == 'C':
        total1 += 100
if 'IV' in user1:
    total1 -= 2 
if 'IX' in user1:
    total1 -= 2 
if 'XL' in user1:
    total1 -= 20

print('The decimal value of', user1, 'is', total1)
print()    
#converting second roman numeral to arabic

user2 = input('Enter second roman numeral: ')

print()

total2 = 0

for i in user2:
    if i == 'I':
        total2 += 1 
    elif i == 'V':
        total2 += 5
    elif i == 'X':
        total2 += 10
    elif i == 'L':
        total2 += 50
    elif i == 'C':
        total2 += 100
if 'IV' in user2:
    total2 -= 2 
if 'IX' in user2:
    total2 -= 2 
if 'XL' in user2:
    total2 -= 20

print('The decimal value of', user2, 'is', total2)
print()

totalSum = total1 + total2

print('The decimal sum is:', totalSum)
print()

numeral = []

# converting from arabic to roman numeral        
# splits the number into integers and appends roman numeral characters to a list 

while totalSum > 1:
    for i in range(len(str(totalSum))): #this loop to identify the one, tens, hundreds place
        if i == 0:
            digit = totalSum % 10 
            totalSum //= 10
            if digit == 9:
                numeral.append('IX')
            if 5 < digit < 9:
                numeral.append('I' * (digit % 5))
                digit -= digit % 5
            if digit == 5: 
                numeral.append('V')
            if digit == 4: 
                numeral.append('IV')
            if digit < 4: 
                numeral.append('I' * digit)
        if i == 1:
            digit = totalSum % 10 
            totalSum //= 10
            if digit == 9:
                numeral.append('XC')
            if 5 < digit < 9:
                numeral.append('X' * (digit % 5))
                digit -= digit % 5
            if digit == 5: 
                numeral.append('L')
            if digit == 4: 
                numeral.append('XL')
            if digit < 4: 
                numeral.append('X' * digit) 
        if i == 2:
            digit = totalSum % 10
            totalSum //= totalSum
            numeral.append('C' * digit)

numeral.reverse()        

print('The roman sum is: ', ''.join(numeral))

Answer 1

There are definitely a few things you could improve here. First, you should create a method to convert a Roman numeral to an Arabic numeral and vice-versa:

def roman_to_arabic(number):

    total = 0

    for i in number:
        if i == 'I':
            total += 1 
        elif i == 'V':
            total += 5
        elif i == 'X':
            total += 10
        elif i == 'L':
            total += 50
        elif i == 'C':
            total += 100
    if 'IV' in number:
        total -= 2 
    if 'IX' in number:
        total -= 2 
    if 'XL' in number:
        total -= 20

    return total

Then, you do not need to write this twice, but can call it like this:

total1 = roman_to_arabic(user1)

Another thing you could do to improve the code is use better names. You have the inputs named as user1 and user2. They do not represent users, but Roman numerals as strings, maybe roman_numeral1 would be a better name. total1 also does not represent a total, but an Arabic numeral converted from a Roman numeral: arabic_numeral1.

Finally, you do not support 'D' (500) or 'M' (1000). If you support these, you should probably add support for "CM" (900) too. Otherwise, your code looks pretty good to me.

Answer 2

Some of the if conditions cannot happen at the same time. For example here:

if 'IV' in user1:
    total1 -= 2 
if 'IX' in user1:
    total1 -= 2 

There cannot be both IV and IX at the same time, so you should chain these conditions using elif, so that the second condition is not evaluated needlessly.

This is even worse when you convert from Roman to Arabic:

if digit == 5: 
    numeral.append('L')
if digit == 4: 
    numeral.append('XL')
if digit < 4: 
    numeral.append('X' * digit)

All these should be elif.

The same goes for this part:

for i in range(len(str(totalSum))): 
    if i == 0:
        digit = totalSum % 10 
        totalSum //= 10
        # ...
    if i == 1:
        digit = totalSum % 10 
        totalSum //= 10
        # ...
    if i == 2:
        digit = totalSum % 10 
        totalSum //= 10
        # ...

But here, there are bigger problems:

• Converting a number to string to get its length is not cool. It's better to repeatedly divide by 10 until you reach 0

• You can actually remove the loop and the if-elif chain, and the code will actually work:

  digit = totalSum % 10 
  totalSum //= 10
  # ...
  
  digit = totalSum % 10 
  totalSum //= 10
  # ...
  
  digit = totalSum % 10 
  totalSum //= 10
  # ...
  

---

It's not good to execute code in global scope. It's recommended to move code inside functions, and execute the main activity of your script from within an if __name__ == '__main__': guard.

Answer 3

Pretty good for your first programming class, but you violate one important principle:

DRY: Do not Repeat Yourself

By putting the conversion code in a function you can reuse it instead of copying it. Furthermore, the conversion code itself is repetitive. You should make better use of loops, because

A programmer does not repeat himself, he loops

In this example, I would take a more data-driven approach such as:

_roman_digits = [
    (1, 'I'),
    (4, 'IV'),
    (5, 'V'),
    (9, 'IX'),
    (10, 'X'),
    (40, 'XL'),
    (50, 'L'),
    (90, 'XC'),
    (100, 'C'),
    (400, 'CD'),
    (500, 'D'),
    (900, 'CM'),
    (1000, 'M'),
][::-1] # process digits from highest to lowest

def roman_to_decimal(roman):
    original = roman
    roman = roman.upper() # allow upper and lower case
    decimal = 0
    for digit in _roman_digits:
        while roman.startswith(digit[1]):
            roman = roman[len(digit[1]):] # discard current digit...
            decimal += digit[0] # ...and add it to result
    if roman: raise ValueError('{} is not a valid roman numeral'.format(original))
    return decimal

def decimal_to_roman(decimal):
    original = decimal
    roman = ''
    for digit in _roman_digits:
        while decimal >= digit[0]:
            decimal -= digit[0] # discard current digit...
            roman += digit[1] # ...and add it to result
    if decimal: raise ValueError('{} could not be converted to a roman numeral'.format(original))
    return roman

Which also shows the similarity between the two conversions much better.

Aside from that, you should try to use meaningful variable names and follow PEP8. Generally it is also recommended to put module-level executing code inside an if __name__ == '__main__':

Furthermore, instead of an additional print() you might want to add newlines to your printed strings instead. Also, you might want to consider using format strings instead of multiple print arguments, as they are more flexible if you want to add quotes or something similar. If you also pack the input and display of the numbers into a function (as it is identical except for the query string passed to input) you might end up with something like

def input_and_display_roman(query):
    roman = input(query)
    decimal = roman_to_decimal(roman)
    print('\nThe decimal value of {} is {}\n'.format(roman, decimal))
    return decimal

if __name__ == '__main__':
    print('Enter two roman numeral number to be added together\n')

    decimal1 = input_and_display_roman('Enter first roman numeral: ')
    decimal2 = input_and_display_roman('Enter second roman numeral: ')
    decimal_sum = decimal1 + decimal2

    print('The decimal sum is: {}\n'.format(decimal_sum))
    roman_sum = decimal_to_roman(decimal_sum)

    print('The roman sum is: {}'.format(roman_sum))