patternpythonMinor
Basic value comparisons
Viewed 0 times
valuebasiccomparisons
Problem
I made a value comparison "programming language" similar to a previous one I made, except this one is based solely on value comparisons.
Here's how you use it.
What can be improved? How can I shorten the code? Any general improvements are welcome. Just don't change the syntax.
# Basic value comparison
from operator import *
# Main compare class
class Compare(object):
def __init__(self):
self.OpError = "OpError: Invalid operator or brace.\n"
self.IntError = "IntError: Invalid integers.\n"
self.StrError = "StrError: Invalid strings.\n"
self.LstError = "LstError: Invalid list.\n"
self.BoolError = "BoolError: Invalid boolean.\n"
self.st_brace = "["
self.end_brace = "]"
self.OPERATORS = {
">=": ge,
"": gt,
"}}>")Here's how you use it.
>}}>cmpbool [ True == True ]
True
>}}>cmplst [ [""] == [""] ]
True
>}}>cmpint [ 2 == 2 ]
True
>}}>cmpstr [ "Hello" == "Hello" ]
TrueWhat can be improved? How can I shorten the code? Any general improvements are welcome. Just don't change the syntax.
Solution
Removing duplicated code
The same code seems to be duplicated in various places. You can easily define a generic function that you'll be able to reuse in all your definitions.
Here's what I have done (not so tested) :
You don't need a class
I suggest you have a look at this Python talk called "Stop Writing Classes ".
In your situation, there is no need for a class at all :
Your code could be written :
Use the logic and the usual programming structure
Instead of writing :
Write :
Also, you could rewrite :
in a way that hilight that sometimes something will be printed, sometimes not.
Define "pure" functions
By defining your functions in such a way that they have no side-effects (printing results for instance), you make them easier to test. In your case, it's quite simple, you can just return instead of printing.
Separe the concerns
You should split the part handling user input/output and the part computing results. This does hand in hand with previous comment.
At this stage, after adding simple tests in the main :
Make things cleaner
It could be a good idea to feed the comparison functions only the information they need : the first element of the list is not relevant anymore at this stage.
You can just write :
and then, you need to re-index
Don't
Style
In Python, the good there is a style guide called PEP 8. You'll find various tools to check your code if you want :
Misc
Usually, using
Also, you should perform a lot more checks before accessing random elements from
The same code seems to be duplicated in various places. You can easily define a generic function that you'll be able to reuse in all your definitions.
Here's what I have done (not so tested) :
def generic_compare(self, command, func, error):
if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
try:
print self.OPERATORS[command[3]](
func(command[2]), func(command[4]))
except ValueError:
print error
if command[3] not in self.OPERATORS:
print self.OpError
# Compare two integers
def compareint(self, command):
self.generic_compare(command, int, self.IntError)
# Compare two strings
def comparestr(self, command):
self.generic_compare(command, eval, self.StrError)
# Compare two lists
def comparelst(self, command):
self.generic_compare(command, eval, self.LstError)
# Compare two booleans
def comparebool(self, command):
self.generic_compare(command, eval, self.BoolError)You don't need a class
I suggest you have a look at this Python talk called "Stop Writing Classes ".
In your situation, there is no need for a class at all :
Your code could be written :
from operator import *
OpError = "OpError: Invalid operator or brace.\n"
IntError = "IntError: Invalid integers.\n"
StrError = "StrError: Invalid strings.\n"
LstError = "LstError: Invalid list.\n"
BoolError = "BoolError: Invalid boolean.\n"
st_brace = "["
end_brace = "]"
OPERATORS = {
">=": ge,
"": gt,
"}}>")Use the logic and the usual programming structure
Instead of writing :
if split_cmd[0] in COMMANDS:
COMMANDS[split_cmd[0]](split_cmd)
if split_cmd[0] not in COMMANDS:
print "CmdError: Invalid command.\n"Write :
if split_cmd[0] in COMMANDS:
COMMANDS[split_cmd[0]](split_cmd)
else:
print "CmdError: Invalid command.\n"Also, you could rewrite :
if command[1] == st_brace and command[3] in OPERATORS and command[5] == end_brace:
try:
print OPERATORS[command[3]](
func(command[2]), func(command[4]))
except ValueError:
print error
if command[3] not in OPERATORS:
print OpErrorin a way that hilight that sometimes something will be printed, sometimes not.
Define "pure" functions
By defining your functions in such a way that they have no side-effects (printing results for instance), you make them easier to test. In your case, it's quite simple, you can just return instead of printing.
Separe the concerns
You should split the part handling user input/output and the part computing results. This does hand in hand with previous comment.
At this stage, after adding simple tests in the main :
from operator import *
OpError = "OpError: Invalid operator or brace.\n"
IntError = "IntError: Invalid integers.\n"
StrError = "StrError: Invalid strings.\n"
LstError = "LstError: Invalid list.\n"
BoolError = "BoolError: Invalid boolean.\n"
st_brace = "["
end_brace = "]"
OPERATORS = {
">=": ge,
"": gt,
"}}>")Make things cleaner
It could be a good idea to feed the comparison functions only the information they need : the first element of the list is not relevant anymore at this stage.
You can just write :
def evaluate_command(command):
split_cmd = command.split(" ")
func, args = split_cmd[0], split_cmd[1:]
if func in COMMANDS:
return COMMANDS[func](args)
else:
return "CmdError: Invalid command.\n"and then, you need to re-index
generic_compare :def generic_compare(command, func, error):
if command[2] in OPERATORS:
if command[0] == st_brace and command[4] == end_brace:
try:
return OPERATORS[command[2]](
func(command[1]), func(command[3]))
except ValueError:
return error
else:
return OpErrorDon't
import *Import * is bad. You can just as easily write :import operator
OPERATORS = {
">=": operator.ge,
"": operator.gt,
"<": operator.lt}Style
In Python, the good there is a style guide called PEP 8. You'll find various tools to check your code if you want :
pep8, pylint, pychecker, pyflakes, etc.Misc
Usually, using
eval in any programming language is quite dangerous. What if I was to evaluate a massive chunk of code with unpredicted side-effects (removing files from my file system for instance) ?Also, you should perform a lot more checks before accessing random elements from
split_cmd as it might be shorter than you expect.Code Snippets
def generic_compare(self, command, func, error):
if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
try:
print self.OPERATORS[command[3]](
func(command[2]), func(command[4]))
except ValueError:
print error
if command[3] not in self.OPERATORS:
print self.OpError
# Compare two integers
def compareint(self, command):
self.generic_compare(command, int, self.IntError)
# Compare two strings
def comparestr(self, command):
self.generic_compare(command, eval, self.StrError)
# Compare two lists
def comparelst(self, command):
self.generic_compare(command, eval, self.LstError)
# Compare two booleans
def comparebool(self, command):
self.generic_compare(command, eval, self.BoolError)from operator import *
OpError = "OpError: Invalid operator or brace.\n"
IntError = "IntError: Invalid integers.\n"
StrError = "StrError: Invalid strings.\n"
LstError = "LstError: Invalid list.\n"
BoolError = "BoolError: Invalid boolean.\n"
st_brace = "["
end_brace = "]"
OPERATORS = {
">=": ge,
"<=": le,
"==": eq,
"!=": ne,
">": gt,
"<": lt}
def generic_compare(command, func, error):
if command[1] == st_brace and command[3] in OPERATORS and command[5] == end_brace:
try:
print OPERATORS[command[3]](
func(command[2]), func(command[4]))
except ValueError:
print error
if command[3] not in OPERATORS:
print OpError
# Compare two integers
def compareint(command):
generic_compare(command, int, IntError)
# Compare two strings
def comparestr(command):
generic_compare(command, eval, StrError)
# Compare two lists
def comparelst(command):
generic_compare(command, eval, LstError)
# Compare two booleans
def comparebool(command):
generic_compare(command, eval, BoolError)
# Dict containing commands
COMMANDS = {
"cmpbool": comparebool,
"cmplst": comparelst,
"cmpint": compareint,
"cmpstr": comparestr,
}
# Read the inputted commands
def read_command(prompt):
command = raw_input(prompt)
split_cmd = command.split(" ")
if split_cmd[0] in COMMANDS:
COMMANDS[split_cmd[0]](split_cmd)
if split_cmd[0] not in COMMANDS:
print "CmdError: Invalid command.\n"
# Run the program
if __name__ == "__main__":
while True: read_command(">}}>")if split_cmd[0] in COMMANDS:
COMMANDS[split_cmd[0]](split_cmd)
if split_cmd[0] not in COMMANDS:
print "CmdError: Invalid command.\n"if split_cmd[0] in COMMANDS:
COMMANDS[split_cmd[0]](split_cmd)
else:
print "CmdError: Invalid command.\n"if command[1] == st_brace and command[3] in OPERATORS and command[5] == end_brace:
try:
print OPERATORS[command[3]](
func(command[2]), func(command[4]))
except ValueError:
print error
if command[3] not in OPERATORS:
print OpErrorContext
StackExchange Code Review Q#61357, answer score: 7
Revisions (0)
No revisions yet.