HiveBrain v1.2.0
Get Started
← Back to all entries
patternpythonMinor

Basic value comparisons

Submitted by: @import:stackexchange-codereview··
0
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.

# 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" ]
True


What 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) :

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 OpError


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 :

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 OpError


Don'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 OpError

Context

StackExchange Code Review Q#61357, answer score: 7

Revisions (0)

No revisions yet.