4
\$\begingroup\$

I'm practicing recursive functions and I would be glad to receive some feedback for the following Python code I wrote. I did several tests and the function yielded correct results but I wonder if it can be written more concise or if it can be criticised in some way. Your feedback would help me become better. The functionality of the function is described in the docstring.

def roman(n):
  """Takes a roman number n as an argument 
     (roman(n) assumes that the string n is 
     correctly written, i.e. it provides no 
     error-checking) and returns an integer."""

  #The base-cases:

  if n == "":
    return 0
  elif n == "M":
    return 1000
  elif n == "D":
    return 500
  elif n == "C":
    return 100
  elif n == "L":
    return 50
  elif n == "X":
    return 10
  elif n == "V":
    return 5
  elif n == "I":
    return 1

  #If a smaller number precedes a bigger number, 
  #then the smaller number is to be subtracted from 
  #the bigger number. Else, it has to be added:

  else:
    if roman(n[0]) < roman(n[1]):
      return (roman(n[1]) - roman(n[0])) + roman(n[2:])
    else:
      return roman(n[0]) + roman(n[1:])

def main(n): 
  print(roman(n))

main("MMMCMXCIX")
\$\endgroup\$
0
6
\$\begingroup\$

Writing down all your basecases takes a lot of space. I would instead use a dictionary, which could even be a global constant.

I find using n as an argument for something that is not an integer to be misleading. A generic x or s or roman_numeral would be clearer, IMO.

Python has an official style-guide, PEP8. It recommends using four spaces as indentation.

BASE_CASES = {"": 0, "M": 1000, "D": 500, "C": 100, "L": 50, "X": 10, "V": 5, "I": 1}

def roman(s):
    """Takes a roman number x as an argument and returns an integer.
    Assumes that the string is correctly written, i.e. it provides no error-checking.
    """
    if s in BASE_CASES:
         return BASE_CASES[s]

    # If a smaller number precedes a bigger number, 
    # then the smaller number is to be subtracted from 
    # the bigger number. Else, it has to be added:
    first, second = map(roman, s[:2])
    if first < second:
        return second - first + roman(s[2:])
    else:
        return first + roman(s[1:])

Of course, using a recursive approach in Python is usually not the best idea, converting it to an iterative approach is much better (i.e. usually faster and not fraught with stack limit issues):

from itertools import tee

def pairwise(iterable):
    "s -> (s0,s1), (s1,s2), (s2, s3), ..."
    a, b = tee(iterable)
    next(b, None)
    return zip(a, b)

def roman(s):
    if s in BASE_CASES:
        return BASE_CASES[s]
    return sum(first if first >= second else second - first
               for first, second in pairwise(map(BASE_CASES.get, s)))
\$\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.