patternpythonMinor

Arabic numbers to Roman numerals and back

Submitted by: @import:stackexchange-codereview·Mar 10, 2026·

Viewed 0 times

romannumbersbacknumeralsandarabic

Problem

I have decided to give python a try and have to start out at the very basics, so I chose this well-represented kata. The code aims to convert Arabic numbers between 1 and 3999 to Roman numerals and back. Here, Roman numerals are 'well-formed', that is, no 'XM' or similar, even though I know they were occasionally used.

I took some of the test values from the publicly available Dive into Python.

I'm very interested in everything you have to say, as from what I've heard Python has quite some guidelines and best practices! Also I'm not sure whether the data structures I use are appropriate/a good choice. But here goes the code:

roman.py

```
import re

class NumberOutOfRangeError(Exception):
pass

class InvalidRomanNumeralError(Exception):
pass

roman_numeral_validator = re.compile(
'^M{0,3}(CM|CD|D?C{0,3})(XC|XL|L?X{0,3})(IX|IV|V?I{0,3})$',
re.IGNORECASE
)

base_conversions = {
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'
}

base_numerals = {v: k for k, v in base_conversions.items()}

base_numbers = sorted(base_conversions.keys(), reverse=True)

def to_roman(arabic_number):
if arabic_number 3999:
raise NumberOutOfRangeError('number is too big')

resulting_numeral = ''
for num in base_numbers:
while arabic_number >= num:
arabic_number -= num
resulting_numeral += base_conversions[num]

return resulting_numeral

def from_roman(roman_numeral):
if not roman_numeral_validator.match(roman_numeral):
raise InvalidRomanNumeralError('invalid roman numeral')

resulting_number = 0
i = 0
while i < len(roman_numeral):
current_letter = roman_numeral[i]

if i + 1 < len(roman_numeral):
potential_base_numeral = current_letter + roman_numeral[i + 1]
if potential_base_numeral in base_numerals:
resulting_num

Solution

Instead of building a dict, sorting the keys and using that sorted list of keys to get the right elements from the dict, why not use a collections.OrderedDict:

from collections import OrderedDict
...
base_conversions = OrderedDict(
    (1000, 'M'),
    ...
    (5, 'V'),
    (4, 'IV'),
    (1, 'I'))

Same for base_numerals.

In python it is considered more pythonic to iterate over the elements of a list. Consider the following implementation where I used zip to have both the current and the following numeral to compare:

def roman_to_int(roman_numeral):
    """Convert from Roman numerals to an integer. Number must be = next_n:
            total += current_n
        else:
            total -= current_n
    return total + numbers[-1]

In addition, you should add a docstring to every function you write. This allows you to do e.g. help(roman_to_int), which will print Convert from Roman numerals to an integer. Number must be < 4000.

Code Snippets

from collections import OrderedDict
...
base_conversions = OrderedDict(
    (1000, 'M'),
    ...
    (5, 'V'),
    (4, 'IV'),
    (1, 'I'))

def roman_to_int(roman_numeral):
    """Convert from Roman numerals to an integer. Number must be < 4000."""
    if not roman_numeral_validator.match(roman_numeral):
        raise InvalidRomanNumeralError('invalid roman numeral')

    numbers = [base_numerals[char] for char in roman_numeral]
    total = 0
    for current_n, next_n in zip(numbers, numbers[1:]):
        if current_n >= next_n:
            total += current_n
        else:
            total -= current_n
    return total + numbers[-1]

Context

StackExchange Code Review Q#138772, answer score: 4

Revisions (0)

No revisions yet.