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.
- 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?
- Would there be a way for me to condense the two
tuples
down into one and still convert correctly?
2 Answers 2
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.
-
1\$\begingroup\$ This should teach me to never assume that if someone posts here, the code actually works. \$\endgroup\$ChatterOne– ChatterOne2016年11月23日 10:41:36 +00:00Commented 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 theif __name__ == '__main__'
? I don't see why that would really make a difference..? \$\endgroup\$papasmurf– papasmurf2016年11月23日 12:12:35 +00:00Commented 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 isVIV
. \$\endgroup\$ChatterOne– ChatterOne2016年11月23日 12:31:50 +00:00Commented 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\$papasmurf– papasmurf2016年11月23日 12:40:36 +00:00Commented Nov 23, 2016 at 12:40
I wouldn't use
if __name__ == '__main__':
for extended logic. I'd put just amain()
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
sayingunrecognized arguments
(which is correct). But if you launch it with no arguments you get the exception.You're checking using
if args.greek:
andelif 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 againstNone
, something likeif args.greek != None:
convert_init
has a parameterconvert_to
with a default ofNone
. Default values are supposed to be either valid or properly handled, and in this caseNone
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 tosymbols_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 ofconvert_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 thatfile_data.write(data)
means# Write it to a file
, no need to comment on that.
Explore related questions
See similar questions with these tags.