10
\$\begingroup\$

I am a beginner, and so I have decided to program a Roman Numeral to Arabic integer converter. I came up with all of the code on my own. There are differing opinions about what constitutes 'proper' Roman Numerals. For example, 'IIII' is odd but known to exist in ancient writings, and 'VIIV' is a silly but parsable way to write '8'. And so my program will treat any valid string as acceptable and attempt to parse it.

It works and I would welcome any comments or considerations, especially with regards to the following:

  • general best practice,
  • whether the input verification can be made cleaner,
  • if there is a nicer way than using a dictionary to match the characters and values,
  • efficiency of the part which calculates the totals, and
  • if I should add comments to the code.
import re

bad = 1                                                                                     

while bad == 1:
    roman = input('Enter a Roman Numeral: ')
    test = re.findall('[^IVXLCDM]+', roman)                                                 
    lengthtracker = 0                                                                       
    for item in test:
        length = len(item)
        lengthtracker = lengthtracker + length                                              
        if length != 0:
            print('Roman Numerals may contain only the characters I, V, X, L, C, D, M')
            break
        else:
            continue
    if lengthtracker == 0:
        bad = 0                                                                             

print('Roman Numeral verified!')                                                            

ref = dict()                                                                                
ref['I'] = 1
ref['V'] = 5
ref['X'] = 10
ref['L'] = 50
ref['C'] = 100
ref['D'] = 500
ref['M'] = 1000

n = len(roman)
m = 0
total = 0
negtotal = 0
tenttotal = 0
while m < n-1:
    if ref[roman[m]] == ref[roman[m+1]]:
        tenttotal = tenttotal + ref[roman[m]]
    elif ref[roman[m]] > ref[roman[m+1]]:
        total = total + tenttotal + ref[roman[m]]
        tenttotal = 0
    else:
        negtotal = negtotal + tenttotal + ref[roman[m]]
        tenttotal = 0
    print(total, negtotal, tenttotal)
    m = m + 1

total = total + tenttotal + ref[roman[m]] - negtotal
    

print('It equals:', total)
\$\endgroup\$
14
\$\begingroup\$

Dict initialization and reuse

This:

ref = dict()                                                                                
ref['I'] = 1
ref['V'] = 5
ref['X'] = 10
ref['L'] = 50
ref['C'] = 100
ref['D'] = 500
ref['M'] = 1000

should use a dict literal:

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

You should declare this at the top of the program, so that this:

'[^IVXLCDM]+'

can become

'[^' + ''.join(ref.keys()) + ']+'

and this

'Roman Numerals may contain only the characters I, V, X, L, C, D, M'

can become

'Roman Numerals may contain only the characters ' + ', '.join(ref.keys())

No-op continue

This:

    else:
        continue

has no effect and can be deleted.

Booleans

bad should use True and False rather than 1 and 0.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for this quick and helpful answer! It seems the main goal of your changes is to save space and be efficient. Am I correct about that? I just want to make sure I get why certain things are best practice. To that end, is the boolean correct you suggest inherently better (and if so, why?) or is it simply just considered best practice? \$\endgroup\$ – The Count Jun 28 at 1:45
  • 4
    \$\begingroup\$ save space and be efficient - sure; basically. Say more with less, and don't repeat yourself while keeping the code legible. \$\endgroup\$ – Reinderien Jun 28 at 2:00
  • 2
    \$\begingroup\$ The boolean is because it more accurately reflects what you're trying to do - represent a yes/no variable, which is what a boolean is designed for. \$\endgroup\$ – Reinderien Jun 28 at 2:00
  • \$\begingroup\$ Thank you very much again! I appreciate you taking the time. \$\endgroup\$ – The Count Jun 28 at 2:01
  • 3
    \$\begingroup\$ I would go a step further and not use the flag bad at all. Use an infinite loop while True, and use break where you would have set bad = 0 to terminate the loop. \$\endgroup\$ – chepner Jun 28 at 15:12
1
\$\begingroup\$

You can simplify foo = foo + ... to just foo += ....

For example you can simplify the following two lines:

m = m + 1
m += 1
total = total + tenttotal + ref[roman[m]] - negtotal
total += tenttotal + ref[roman[m]] - negtotal
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Thank you for these hints! Definitely a space saver. \$\endgroup\$ – The Count Jun 28 at 16:46
  • 1
    \$\begingroup\$ Liked it that I could be helpful. \$\endgroup\$ – Vishesh Mangla Jun 28 at 16:49
1
\$\begingroup\$

The algorithm you're using is excessively complex. There's no need to track total and negtotal separately. You also don't need tenttotal unless you want to support particularly exotic numerals like IIX (which is rare but apparently not unattested). And if you're not tracking tenttotal, then you don't need to distinguish the "current = next" and "current > next" cases, either.

The algorithm I'd use is:

  • Start with total = 0.
  • For each character in the string:
    • If the current character's value is less than the following character's value, subtract the current character's value from the total.
    • Otherwise, add the current character's value to the total.

So, something like this:

for i in range(len(roman)):
    current_char_value = ref[roman[i]]
    next_char_value = ref[roman[i+1]] if i+1 < len(roman) else 0

    if current_char_value < next_char_value:
        total -= current_char_value
    else:
        total += current_char_value

By the way, ref is a strange name for your dictionary. letter_values would make a lot more sense.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ I do want to support things like 'IIX', I fear. I did try this algorithm at first. \$\endgroup\$ – The Count Jun 28 at 17:02

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.