4
\$\begingroup\$

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,
            "<=": le,
            "==": eq,
            "!=": ne,
            ">": gt,
            "<": lt}

    # Compare two integers
    def compareint(self, command):
        if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
            try:
                print self.OPERATORS[command[3]](
                    int(command[2]), int(command[4]))
            except ValueError:
                print self.IntError
        if command[3] not in self.OPERATORS:
            print self.OpError

    # Compare two strings
    def comparestr(self, command):
        if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
            try:
                print self.OPERATORS[command[3]](
                    eval(command[2]), eval(command[4]))
            except SyntaxError:
                print self.StrError
        if command[3] not in self.OPERATORS:
            print self.OpError

    # Compare two lists
    def comparelst(self, command):
        if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
            try:
                print self.OPERATORS[command[3]](
                    eval(command[2]), eval(command[4]))
            except SyntaxError:
                print self.LstError
        if command[3] not in self.OPERATORS:
            print self.OpError

    # Compare two booleans
    def comparebool(self, command):
        if command[1] == self.st_brace and command[3] in self.OPERATORS and command[5] == self.end_brace:
            try:
                print self.OPERATORS[command[3]](
                    eval(command[2]), eval(command[4]))
            except NameError:
                print self.BoolError
        if command[3] not in self.OPERATORS:
            print self.OpError


# Dict containing commands
COMMANDS = {
    "cmpbool": Compare().comparebool,
    "cmplst": Compare().comparelst,
    "cmpint": Compare().compareint,
    "cmpstr": Compare().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(">}}>")

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.

\$\endgroup\$
7
\$\begingroup\$

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,
        "<=": 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(">}}>")

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,
        "<=": le,
        "==": eq,
        "!=": ne,
        ">": gt,
        "<": lt}

def generic_compare(command, func, error):
    if command[3] in OPERATORS:
        if command[1] == st_brace and command[5] == end_brace:
            try:
                return OPERATORS[command[3]](
                    func(command[2]), func(command[4]))
            except ValueError:
                return error
    else: 
        return OpError

# Compare two integers
def compareint(command):
    return generic_compare(command, int, IntError)

# Compare two strings
def comparestr(command):
    return generic_compare(command, eval, StrError)

# Compare two lists
def comparelst(command):
    return generic_compare(command, eval, LstError)

# Compare two booleans
def comparebool(command):
    return generic_compare(command, eval, BoolError)

# Dict containing commands
COMMANDS = {
    "cmpbool": comparebool,
    "cmplst": comparelst,
    "cmpint": compareint,
    "cmpstr": comparestr,
}


def evaluate_command(command):
    split_cmd = command.split(" ")
    if split_cmd[0] in COMMANDS:
        return COMMANDS[split_cmd[0]](split_cmd)
    else:
        return "CmdError: Invalid command.\n"

# Read the inputted commands
def read_command(prompt):
    print(evaluate_command(raw_input(prompt)))


# Run the program
if __name__ == "__main__":
    assert evaluate_command('cmpbool [ True == True ]')
    assert not evaluate_command('cmpbool [ True == False ]')
    assert evaluate_command('cmplst [ [""] == [""] ]')
    assert not evaluate_command('cmplst [ [""] == ["a"] ]')
    assert evaluate_command('cmpint [ 2 == 2 ]')
    assert not evaluate_command('cmpint [ 2 == 3 ]')
    assert evaluate_command('cmpstr [ "Hello" == "Hello" ]')
    assert not evaluate_command('cmpstr [ "Hello" == "HelloW" ]')
    #while True: read_command(">}}>")

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.le,
        "==": operator.eq,
        "!=": operator.ne,
        ">":  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.

\$\endgroup\$
6
\$\begingroup\$

Your code isn't fully PEP-8 (the Python style guide) compliant. Function names should be lowercase_with_underscores.

You also have a lot of duplicated code. I would suggest pulling out the common components and making it a generic function. I basically just pulled out the core logic, added a type parameter, and added in separate cases for int, str, and etc. I got something like this:

def compare_value(self, command, valueType):
    operator = command[3]
    if command[1] == self.st_brace and operator in self.OPERATORS and command[5] == self.end_brace:
        a = command[2]
        b = command[4]
        if valueType == int:
            # pass
        elif valueType == str:
            # pass
        elif valueType == bool:
            # pass
        elif valueType == list:
            # pass
    if operator not in self.OPERATORS:
        print self.OpError

# Compare two integers
def compareint(self, command):
    self.compare_value(command, int)

# Compare two strings
def comparestr(self, command):
    self.compare_value(command, str)

# Compare two lists
def comparelst(self, command):
    self.compare_value(command, list)

# Compare two booleans
def comparebool(self, command):
    self.compare_value(command, bool)
\$\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.