patternpythonMinor
Adding two roman-numeral inputs
Viewed 0 times
inputsromanaddingtwonumeral
Problem
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
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
Solution
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:
Then, you do not need to write this twice, but can call it like this:
Another thing you could do to improve the code is use better names. You have the inputs named as
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.
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.
Context
StackExchange Code Review Q#80496, answer score: 8
Revisions (0)
No revisions yet.