4
\$\begingroup\$

Yesterday I posted a question about converting an integer to it's Roman Numeral equivalent. Apparently you guys liked it and gave me some good advice, so I decided to take all your advice, and go a step further, it will now not only convert to Roman but will now also convert to Greek numerals.

I would like some critique on this project that I'm continuing.

# <~~ coding=utf-8 ~~>
import argparse
opts = argparse.ArgumentParser()
opts.add_argument("-g", "--greek", type=int,
 help="Convert to Medieval Numerals")
opts.add_argument("-r", "--roman", type=int,
 help="Convert to Roman Numerals")
args = opts.parse_args()
ROMAN_NUMERAL_TABLE = (
 ("~M", 1000000), ("~D", 500000), ("~C", 100000),
 ("~L", 50000), ("~X", 10000), ("~V", 5000), # "~" indicates a Macron
 ("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)
)
GREEK_NUMERAL_TABLE = (
 ("α", 1), ("β", 2), ("γ", 3),
 ("δ", 4), ("ε", 5), ("Ϝ", 6),
 ("ζ", 7), ("η", 8), ("θ", 9),
 ("ι", 10), ("κ", 20), ("λ", 30),
 ("μ", 40), ("ν", 50), ("ξ", 60),
 ("ο", 70), ("π", 80), ("ϙ", 90),
 ("ρ", 100), ("σ", 200), ("τ", 300),
 ("υ", 400), ("φ", 500), ("χ", 600),
 ("ψ", 700), ("ω", 800), ("ϡ", 900),
 ("α", 1000), ("β", 2000), ("γ", 3000),
 ("δ", 4000), ("ε", 5000), ("ϛ", 6000),
 ("ζ", 7000), ("η", 8000), ("θ", 9000) # The Greeks weren't very creative
)
def convert_init(number, convert_to=None):
 """ Convert a number to a numeral, Greek or Roman
 >>> print(convert_init(45, convert_to=GREEK_NUMERAL_TABLE))
 ϜϜϜϜϜϜϜγ
 >>> print(convert_init(45, convert_to=ROMAN_NUMERAL_TABLE))
 XLV """
 display_numerals = []
 for numeral, value in sorted(convert_to)[::-1]: # sort the list from largest to least
 count = number // value
 number -= count * value
 display_numerals.append(numeral * count)
 return ''.join(display_numerals)
if __name__ == '__main__':
 if args.greek:
 data = convert_init(int(args.greek), convert_to=GREEK_NUMERAL_TABLE)
 with open("greek_numerals.txt", "a+") as file_data:
 file_data.write(data) # Write it to a file
 elif args.roman:
 data = convert_init(int(args.roman), convert_to=ROMAN_NUMERAL_TABLE)
 with open("roman_numerals.txt", "a+") as file_data:
 file_data.write(data)
 else:
 raise NotImplementedError("{} is not implemented yet".format(args))

Key points I would like to focus on, of course critique the whole program.

  1. I used writing to a file because when if I output it to the console I get an IO error, basically an encoding error. Does this have to do with my computer or with the program?
  2. Would there be a way for me to condense the two tuples down into one and still convert correctly?
asked Nov 22, 2016 at 14:58
\$\endgroup\$

2 Answers 2

4
\$\begingroup\$

Your conversion routines make no sense. I don't see why 45 should be ϜϜϜϜϜϜϜγ, as stated in your doctest. I also get this clearly wrong result:

>>> convert_init(1939, ROMAN_NUMERAL_TABLE)
'XLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXLXVIV'

Since you are using Unicode for the Greek numerals, I should be pedantic and point out that the Roman numerals should also use the appropriate Unicode characters.

The docstring would be better written as

def convert_init(number, convert_to=None):
 """
 Convert a number to a Greek or Roman numeral
 >>> convert_init(45, convert_to=ROMAN_NUMERAL_TABLE)
 'XLV'
 """
 ...

... since the print statement obscures the fact that you are returning a string. (Based on just your docstring, it could also be returning some other kind of object, whose __str__ produces the desired output.)

What is convert_init? Did you mean convert_int?

Your args = opts.parse_args() is not guarded by if __name__ == '__main__' as it should be. You shouldn't need to cast int(args.greek) or int(args.roman), since argparse should take care of the type=int for you.

answered Nov 23, 2016 at 9:40
\$\endgroup\$
4
  • 1
    \$\begingroup\$ This should teach me to never assume that if someone posts here, the code actually works. \$\endgroup\$ Commented Nov 23, 2016 at 10:41
  • \$\begingroup\$ 1. @ChatterOne It does work. It just doesn't work as I assumed it would. 2. @200_success I got all the information for the Greek letters here, are you telling me they're wrong? 3. Docstring is a valid critique, thank you. 4. Why does argparse have to be in the if __name__ == '__main__'? I don't see why that would really make a difference..? \$\endgroup\$ Commented Nov 23, 2016 at 12:12
  • 2
    \$\begingroup\$ @papasmurf If you say that your program doesn't work as you assumed it would, that's the same as saying there's a bug. So, no, it doesn't work. An example: the number 9 in roman numerals is IX, your output is VIV. \$\endgroup\$ Commented Nov 23, 2016 at 12:31
  • \$\begingroup\$ @ChatterOne Touche sir. Guess it's got some bugs in it lol. I'll fix them, thank you guys for bringing these to my attention \$\endgroup\$ Commented Nov 23, 2016 at 12:40
3
\$\begingroup\$
  • I wouldn't use if __name__ == '__main__': for extended logic. I'd put just a main() in there and do the checks/call the functions from there.

  • Your script is supposed to have user input and output. Having an exception is a nice way to communicate between modules/functions, but with the user you may prefer a print()

  • The behavior is not entirely consistent because launching the program with an unknown option triggers a message from argparse saying unrecognized arguments (which is correct). But if you launch it with no arguments you get the exception.

  • You're checking using if args.greek: and elif args.roman:, but if you give a false value, they will fail. That means that passing -r 0 on the command line will trigger the exception of not implemented. You should check against None, something like if args.greek != None:

  • convert_init has a parameter convert_to with a default of None. Default values are supposed to be either valid or properly handled, and in this case None is ... well, none of the two.

  • convert_to is not a proper name, because it suggests that there are two possible values, roman and greek, while in reality that's the lookup table you're going to use. I'd either rename it to symbols_lookup or (better IMO), keep the name and use it to determine which lookup table to use inside of the function.

  • In convert_init you sort the values of convert_to, but they're static values, so you can sort them beforehand and use the values directly.

  • You have a comment saying # "~" indicates a Macron which is useful. The other comments can (and should) go. You know that file_data.write(data) means # Write it to a file, no need to comment on that.

answered Nov 23, 2016 at 8:35
\$\endgroup\$

Your Answer

Draft saved
Draft discarded

Sign up or log in

Sign up using Google
Sign up using Email and Password

Post as a guest

Required, but never shown

Post as a guest

Required, but never shown

By clicking "Post Your Answer", you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.