5
\$\begingroup\$

I recently started learning python.

Requirements

  • Input: Integer between 1 and 3999
  • Output: String with Roman numerals

I would appreciate it if someone could have look at the code and give me any feedback. I am particularly interested in the Python specific improvements, as I mainly programmed in C before.

Edit: Any feedback on the algorithm is appreciated too.

import sys

symbols1 =     [(1,   "I"), (5,   "V"), (10,   "X")]
symbols10 =    [(10,  "X"), (50,  "L"), (100,  "C")]
symbols100 =   [(100, "C"), (500, "D"), (1000, "M")]

symList = [symbols1, symbols10, symbols100]

def digitToRomanSym (digit, symList):
    """ Converts a single digit into its roman symbols """
    one =  symList[0][1]
    five = symList[1][1]
    ten =  symList[2][1]

    switcher = {
        1: one,
        2: one + one,
        3: one + one + one,
        4: one + five,
        5: five,
        6: five + one,
        7: five + one + one,
        8: five + one + one + one,
        9: one + ten,
    }
    return switcher.get(digit, "")

def getRomanNumerals (n):
    """ Converts a number between 1 and 3999 into Roman numerals """
    maxNumber = symList[-1][-1][0] * 4 - 1 # A symbol can be repeated max 3 times e.g. MMM for 3000

    if (n > maxNumber):
        return "Number too big"

    if (n == 0):
        return "Number too small"

    i = 0
    romanStr = ""
    while (n > 0 and i < len(symList)):
        romanStr = digitToRomanSym(n % 10, symList[i]) + romanStr
        n /= 10
        i += 1

    if (n > 0):
        biggestSymbol = symList[-1][-1][1]
        romanStr = (n * biggestSymbol) + romanStr;

    return romanStr

if (len(sys.argv) == 2):
    number = int(sys.argv[1])
    print(getRomanNumerals(number))
else:
    print("Wrong number of arguments")
\$\endgroup\$
  • 2
    \$\begingroup\$ Python 2 actually. It relies on /= doing integer division in getRomanNumerals, when doing n /= 10. Making it n //= 10 gets it to work in Python 3.x. \$\endgroup\$ – Graipher Jan 17 '17 at 13:58
  • 1
    \$\begingroup\$ You might want to have a look at the answer (by me) to this similar question: codereview.stackexchange.com/q/138772/98493 \$\endgroup\$ – Graipher Jan 17 '17 at 14:00
3
\$\begingroup\$

Welcome to Code Review!

Most of my suggestions are stylistic, but still something to care about in the Python world. (PEP8 Style Guide) I did catch a fairly major bug since you are only accepting 1 - 3999 as valid numbers, so we fix that too.


Redundant Parentheses

Unlike in C or C++ (or even Java to some level) you don't have to wrap conditionals in parentheses typically. There are some cases where you would need to, but your if conditions are not complex enough to need that.


BUG: If an integer less than 0 is passed, there is no warning about being too small a number

Therefore, if a negative number is passed in at runtime, it outputs nothing and no errors. We can fix this in one line of code - we just change your == check to a <= check (less than or equal to):

if num <= 0:
    return "Number too small"

Descriptive Parameters

Normally, you should name an parameter for a function in a more descriptive way. While n can be assumed to be a number, it would be better to have num instead of n as an parameter name for getRomanNumerals to better describe what that is.

My two cents, of course, but in more complex functions where n could lose meaning, you'd be better off with a more descriptive parameter name so you don't lose track of what n does.


Rogue Semicolon!

On line 47, you have a rogue trailing semicolon. While this doesn't affect running of your program, such semicolons are not necessary.


Style: Function names should be lowercase; multiword names separated with underscores

From the PEP8 (linked above):

Function names should be lowercase, with words separated by underscores as necessary to improve readability.

mixedCase is allowed only in contexts where that's already the prevailing style (e.g. threading.py), to retain backwards compatibility.

Therefore, your function names should be renamed as follows:

  • digitToRomanSym becomes digit_to_roman_sym
  • getRomanNumerals becomes get_roman_numerals

Style: Parameter variable names usually are lowercase

While not explicitly defined in PEP8 as far as I can tell, most of the time, arguments within a function (for example, symList in digitToRomanSym in your code) tend to be lowercase and if multiword, are underscore-separated.


Style: Local variable names should be lowercase, multiword separated with underscores

Just like function definitions, local variables should be lowercase. That is, variables within the scope of a single function.



Your code after my changes:

(Filename: int2roman.py)

import sys

symbols1 =     [(1,   "I"), (5,   "V"), (10,   "X")]
symbols10 =    [(10,  "X"), (50,  "L"), (100,  "C")]
symbols100 =   [(100, "C"), (500, "D"), (1000, "M")]

symList = [symbols1, symbols10, symbols100]

def digit_to_roman_sym (digit, sym_list):
    """ Converts a single digit into its roman symbols """
    one =  sym_list[0][1]
    five = sym_list[1][1]
    ten =  sym_list[2][1]

    switcher = {
        1: one,
        2: one + one,
        3: one + one + one,
        4: one + five,
        5: five,
        6: five + one,
        7: five + one + one,
        8: five + one + one + one,
        9: one + ten,
    }
    return switcher.get(digit, "")

def get_roman_numerals(num):
    """ Converts a num between 1 and 3999 into Roman numerals """
    max_number = symList[-1][-1][0] * 4 - 1 # A symbol can be repeated max 3 times e.g. MMM for 3000

    if num > max_number:
        return "Number too big"

    if num <= 0:
        return "Number too small"

    i = 0
    roman_str = ""
    while num > 0 and i < len(symList):
        roman_str = digit_to_roman_sym(num % 10, symList[i]) + roman_str
        num /= 10
        i += 1

    if num > 0:
        biggest_symbol = symList[-1][-1][1]
        roman_str = (num * biggest_symbol) + roman_str

    return roman_str

if len(sys.argv) == 2:
    number = int(sys.argv[1])
    print(get_roman_numerals(number))
else:
    print("Wrong number of arguments")
\$\endgroup\$
  • \$\begingroup\$ Thanks for the review, I will look at it in detail later today. Just a quick question, is camelcase not pythonic? \$\endgroup\$ – Frode Akselsen Jan 17 '17 at 21:32
  • 1
    \$\begingroup\$ @FrodeAkselsen No sense CamelCase is part of PEP8 \$\endgroup\$ – Anthony Pham Jan 17 '17 at 22:11

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.