6
\$\begingroup\$

I got sent a challenge from a friend;

You are given a file that contains numbers, one number per line. Convert the numbers from the file into roman numerals.

Test input:
45
65
23
98
123
7754

Expected Output:
XLV
LXV
XXIII
XCVIII
CXXIII
MMMMMMMDCCLIV

I think I've succeeded in doing this successfully, and would like some critque on what I've done so far:

import sys


ROMAN_NUMERAL_TABLE = [
    ("M", 1000), ("CM", 900), ("D", 500),
    ("CD", 400), ("C", 100),  ("XC", 90),
    ("L", 50),   ("XL", 40),  ("X", 10),
    ("IX", 9),   ("V", 5),    ("IV", 4),
    ("I", 1)
]


def convert_to_roman(number):
    """ Convert an integer to Roman
    >>> print(convert_to_roman(45))
    XLV """
    roman_numerals = []
    for numeral, value in ROMAN_NUMERAL_TABLE:
        while value <= number:
            number -= value
            roman_numerals.append(numeral)

    return ''.join(roman_numerals)


if __name__ == '__main__':
    with open(sys.argv[1]) as data:
        for line in data.readlines():
            print("{} to roman numerals: {}".format(line.rstrip(), convert_to_roman(int(line.rstrip()))))

Key points that I would like to focus on (obviously feel free to critique the entire program)

  1. Using the tuples for the list, is this good practice, or would a dict have worked better?
  2. While converting the integers to the roman numerals is there a easier way to do this, right now my program will take a long time, for an integer that is higher than 12345679990 is there a way to speed up the processing for extremely high integers?
\$\endgroup\$
5
\$\begingroup\$
  1. Using the tuples for the list, is this good practice, or would a dict have worked better?

Yes, in this case I think it's ok. Alternative is using orderdict or dict but sorting it by value which is not that good.

Now to make it work faster you can use division and multiplication. So it will look like this:

def convert_to_roman(number):
    """ Convert an integer to Roman
    >>> print(convert_to_roman(45))
    XLV """
    roman_numerals = []
    for numeral, value in ROMAN_NUMERAL_TABLE:
        count = number // value
        number -= count * value
        roman_numerals.append(numeral * count)

    return ''.join(roman_numerals)
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ Alternatively, count, number = divmod(number, value) \$\endgroup\$ – 301_Moved_Permanently Nov 22 '16 at 8:04
  • \$\begingroup\$ @MathiasEttinger What is divmod()? \$\endgroup\$ – papasmurf Nov 22 '16 at 12:04
  • 1
    \$\begingroup\$ @papasmurf help(divmod) \$\endgroup\$ – 301_Moved_Permanently Nov 22 '16 at 12:54
2
\$\begingroup\$

Using the tuples for the list, is this good practice, or would a dict have worked better?

I would have just used a tuple instead of a list to enforce immutability.

roman_numeral_table = (
    ("M", 1000), ("CM", 900), ("D", 500), ("CD", 400), ("C", 100), ("XC", 90),
    ("L", 50),   ("XL", 40),  ("X", 10),  ("IX", 9),   ("V", 5),   ("IV", 4),
    ("I", 1)
)

You should also check if the number you are converting is valid.

class ToRomanError(Exception): pass
class OutOfRangeError(ToRomanError): pass
class NotIntegerError(ToRomanError): pass

def convert_to_roman(number):
    if not (0 < number):
        raise OutOfRangeError, "number must be non-negative"
    if int(number) != number
        raise NotIntegerError, "cannot convert decimals"
| improve this answer | |
\$\endgroup\$
  • 1
    \$\begingroup\$ if not (0 < number) is a lot harder to understand than if number <= 0. \$\endgroup\$ – Graipher Nov 22 '16 at 6:42

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.