4
\$\begingroup\$

Below is my code for the problem:

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

def convertItoR(number):
    number_str = str(number)
    number_len = len(number_str)


    rom_str = ''
    for i in range(number_len):
        # Extracting the digit
        digit = int(number_str[i])
        position = number_len - i - 1
        num_pos = digit * 10 ** position

        print(num_pos)

        # Converting digit to roman string
        if num_pos in ItoR.keys():
            rom_str += ItoR[num_pos]

        elif digit > 1 and digit < 4:
            first = 1 * 10 ** position
            rom_str += ItoR[first] * digit
        
        elif digit > 5 and digit < 9:
            first = 1 * 10 ** position
            fifth = 5 * 10 ** position
            rom_str += ItoR[fifth] + ItoR[first] * (digit - 5) 

        print(rom_str)

    return rom_str

Can it be optimized?

\$\endgroup\$
3
\$\begingroup\$

You asked about optimization, which usually refers either to speed or memory usage, neither of which apply in this case. Your code has negligible memory cost. And if we truly cared about the ability to convert large volumes of numbers at high speed, we would pre-compute a dict mapping every decimal number of interest to its Roman equivalent.

If I were you, I would focus on optimizing this problem for simplicity of understanding and code readability. Your current code appears to be fine, and one can definitely figure it out, but it still requires some puzzling over. The classic way that I've seen people solve this problem more simply is to build up the Roman numeral by subtracting away even multiples of each divisor in your dict mapping divisors to Roman symbols. The trick is to proceed from largest divisor to smallest. Since Python 3 preserves order in dicts, we can achieve that simply by reversing the order in which the data is set up. The basic idea is illustrated below.

def decimal_to_roman(num):
    # The Romans had "integers" too, so this seems like a more
    # accurate function name.

    # You should add a check here to validate that the given decimal
    # number falls within the supported ranged.

    # Relies on Python 3 having ordered dicts.
    divisors = {
        1000: 'M',
        900: 'CM',
        500: 'D',
        400: 'CD',
        100: 'C',
        90: 'XC',
        50: 'L',
        40: 'XL',
        10: 'X',
        9: 'IX',
        5: 'V',
        4: 'IV',
        1: 'I',
    }

    # Build up the Roman numeral by subtracting multiples of
    # each divisor, proceeding from largest to smallest.
    roman = ''
    for div, sym in divisors.items():
        quotient = divmod(num, div)[0]
        roman += sym * quotient
        num -= div * quotient
        if num < 1:
            break
    return roman

When deciding on names for functions, variables, and so forth, aim for accuracy and clarity in situations where the additional context is needed for readability. This is a case where slightly longer names are appropriate (for example, decimal_to_roman, divisors, and quotient). But when the surrounding context and nearby names provide sufficient clues, you can aim for brevity to reduce the visual weight of the code -- which also helps with readability, but from a different direction. For example, within the narrow context of this function, the short names like num, div, and sym are easily understood without giving them fully explicit names.

Finally, how do you know your code is correct? If you're trying to learn, you could consider writing the corresponding roman_to_decimal() function and then make sure your converters can round-trip the data correctly. Another approach is to find one of the many Python functions floating around the internet and use it as your checker. Or you can install the roman Python package to check everything, as shown here:

import roman

for n in range(1, 4000):
    exp = roman.toRoman(n)
    got = decimal_to_roman(n)
    if exp != got:
        print(n, exp, got)
\$\endgroup\$
2
\$\begingroup\$

maybe if you introduce a layer for positioning to your map, you could make the code slightly more readable (use the position instead of calculating map index):

ItoR = {
1: {1: 'I',
    4: 'IV',
    5: 'V',
    9: 'IX'},
2: {
    1: 'X',
    4: 'XL',
    5: 'L',
    9: 'XC'},
...
}
\$\endgroup\$

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.