2
\$\begingroup\$

The goal of this calculator is to evaluate an expression with PEMDAS in mind (no parenthesis yet, adding soon). Although the program works, I want to make it as efficient as possible with the approach I used to attack the mathematical expression.

So the way I decided to approach the problem is to initially organize the expression via my cleanUp() so that it's easier to manipulate/iterate afterwards, for example:

"2+2 -4 * 1 / 3.344 - -2" becomes "2 + 2 - 4 * 1 / 3.344 + 2" which can now be easily split by a space and the calculation process is now a lot easier, but I feel like my cleanUp() function can be a lot more efficient. (I'm still learning Time complexities of built-in functions of Python 3)

Here is my whole code, feel free to comment on any other part of the program, but I'm mostly asking for optimization improvements on the cleanUp() function.

import time

def isNumber(s):
    #s must be a non-empty string
    #returns True if it's convertible to float, else False
    if (len(s)==0 and not s =='') or not isinstance(s, str):
        print("type mismatch error: isNumber")
        return "type mismatch error: isNumber"
    try:
        s = float(s)
        return True
    except ValueError:
        #print("type mismatch error: isNumber")
        return False


def cleanUp(s):
    priorSpace = True
    inDecimal = False
    ops = ["+","-","*","/","^"]
    s = s.replace("+", " + ").replace("-", " - ").replace("*", " * ").replace("/", " / ").replace("^", " ^ ")
    st = list(s)
    for i,c in enumerate(s):
        if (not isNumber(c) and c not in ops) and priorSpace:
            if c == "." and not inDecimal:
                inDecimal = True
            elif c == "." and inDecimal:
                st[i] = ""
            else:
                st[i] = ""
                inDecimal = False
        elif (not isNumber(c) and c not in ops) and not priorSpace:
            if c == "." and not inDecimal:
                inDecimal = True
            elif c == "." and inDecimal:
                st[i] = ""
            else:
                st[i] = " "
                priorSpace = True
                inDecimal = False
        else:
            priorSpace = False
    final = ''.join(st).strip().split(' ')
    if '-' in final[0]:
        del final[0]
        final[0] = '-' + final[0]
    newNumber, newOpr, oprPos = getNextNumber(final, 0)
    if oprPos == None:
        return newNumber
    cacheNum=newNumber
    pos=oprPos+1                #the new current position
    opr=newOpr
    while pos<(len(final)-1):
        newNumber, newOpr, oprPos = getNextNumber(final, pos)
        if (newOpr == None):
            break
        if (not isNumber(newNumber) and newNumber == '-'):
            del final[oprPos-1] #BOOKMARK
            final[oprPos-1] = '-' + final[oprPos-1]
        if (not isNumber(newNumber) and newNumber != '-'):
            print("error at line AB", newNumber)
            return "error at line AB"
        pos=oprPos+1
    return (final)


def getNextNumber(expr, pos):
    #expr is a given arithmetic formula in string
    temp = expr
    #pos = start position in expr
    #1st returned value = the next number (None if N/A)
    #2nd returned value = the next operator (None if N/A)
    #3rd retruned value = the next operator position (None if N/A)
    ops = ["+","-","*","/"]
    if len(expr)==0 or pos<0 or pos>=len(expr) or not isinstance(pos, int):
        print("type mismatch error: getNextNumber")
        return None, None, "type mismatch error: getNextNumber"
    #--- function code starts ---#
    res3 = 0
    res2 = 0
    if (pos+1) < len(temp):
        res3 = pos+1
        res2 = temp[pos+1]
    else:
        res3 = None
        res2 = None
    return ((temp[pos], res2, res3))



    #--- function code ends ---#


def exeOpr(num1, opr, num2):
    #This is a simple utility function skipping type check
    if opr=="+":
        return num1+num2
    elif opr=="-":
        return num1-num2
    elif opr=="*":
        return num1*num2
    elif opr=="/":
        return num1/num2
    elif opr=="^":
        return num1**num2
    else:
        return None


def calc(expr):
    #expr: nonempty string that is an arithmetic expression
    #the fuction returns the calculated result
    expr = cleanUp(expr)
    if len(expr)<=0:
        print("argument error: line A in eval_expr")        #Line A
        return "argument error: line A in eval_expr"
    #Hold two modes: "addition" and "multiplication"
    #Initializtion: get the first number
    newNumber, newOpr, oprPos = getNextNumber(expr, 0)
    ops = ["^","*","/","+","-"]
    if newNumber is None or not isNumber(newNumber):
        print("input formula error: line B in eval_expr")   #Line B
        return "input formula error: line B in eval_expr"
    elif newOpr is None or not newOpr in ops:
        return newNumber
    cacheNum=newNumber
    pos=oprPos+1                #the new current position
    opr=newOpr                  #the new current operator
    #start the calculation. Use the above functions effectively.
    new_expr = expr
    while True:
        newNumber, newOpr, oprPos = getNextNumber(new_expr, pos)
        #print(cacheNum, opr, newNumber, "The next opr however is", newOpr)
        if (not isNumber(newNumber)):
            print("input formula error: line C in eval_expr")   #Line C
            return "input formula error: line C in eval_expr"
        if (newOpr == None):
            break
        if(newOpr == "^"):
            temp = getNextNumber(new_expr, oprPos+1)[0]
            if (not isNumber(temp)):
                print("input formula error: line D in eval_expr")   #Line D
                return "input formula error: line D in eval_expr"
            #print("Weee", newOpr, newNumber, "with", temp)
            subs = exeOpr(float(newNumber), newOpr, float(temp))
            del new_expr[oprPos-1]
            del new_expr[oprPos-1]
            new_expr[oprPos-1] = str(subs)
            #print("The new expression to start calculating is: " + new_expr)
        elif((newOpr == "*" or newOpr == "/") and (opr not in ["^","*","/"])):
            temp, temp_Opr, temp_Pos = getNextNumber(new_expr, oprPos+1)
            #print("uhhh", temp_Opr)
            if temp_Opr == "^":
                temp2 = getNextNumber(new_expr, oprPos+3)[0]
                if (not isNumber(temp)):
                    print("input formula error: line D in eval_expr")   #Line D
                    return "input formula error: line D in eval_expr"
                #print("We", temp_Opr, temp, "with", temp2)
                subs = exeOpr(float(temp), temp_Opr, float(temp2))
                del new_expr[temp_Pos-1]
                del new_expr[temp_Pos-1]
                new_expr[temp_Pos-1] = str(subs)
                #print("The new expression to start calculating is: " + new_expr)
            else:
                if (not isNumber(temp)):
                    print("input formula error: line D in eval_expr")   #Line D
                    return "input formula error: line D in eval_expr"
                #print("Weee", newOpr, newNumber, "with", temp)
                subs = exeOpr(float(newNumber), newOpr, float(temp))
                del new_expr[oprPos-1]
                del new_expr[oprPos-1]
                new_expr[oprPos-1] = str(subs)
                #print("The new expression to start calculating is: " + new_expr)
        elif((newOpr == "+" or newOpr == "-") and opr not in ["+","-"]):
            subs = exeOpr(float(cacheNum), opr, float(newNumber))
            del new_expr[pos-2]
            del new_expr[pos-2]
            new_expr[pos-2] = str(subs)
            cacheNum=str(subs)
            opr=newOpr
            #print("The new expressionn to start calculating is: " + new_expr)
        else:
            subs = exeOpr(float(cacheNum), opr, float(newNumber))
            del new_expr[pos-2]
            del new_expr[pos-2]
            new_expr[pos-2] = str(subs)
            #print("The new expression to start calculating is: " + new_expr)
            cacheNum=str(subs)
            opr=newOpr
    subs = exeOpr(float(cacheNum), opr, float(newNumber))
    #print(new_expr)
    del new_expr[new_expr.index(cacheNum)]
    del new_expr[new_expr.index(opr)]
    new_expr[new_expr.index(newNumber)] = str(subs)
    return float(new_expr[0])

print(calc("2+2 -4 * 1 / 3.344 - -2")
\$\endgroup\$

closed as off-topic by Vogel612, Dannnno, Donald.McLean, ChrisWue, Graipher Sep 28 '17 at 14:13

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Questions containing broken code or asking for advice about code not yet written are off-topic, as the code is not ready for review. After the question has been edited to contain working code, we will consider reopening it." – Vogel612, Dannnno, Donald.McLean, ChrisWue, Graipher
If this question can be reworded to fit the rules in the help center, please edit the question.

  • 1
    \$\begingroup\$ Your last line is missing a ) at the end. \$\endgroup\$ – Richard Neumann Sep 27 '17 at 11:17
  • \$\begingroup\$ @Ludisposed Fixed it to work, lemme know if you still run into it. \$\endgroup\$ – Blanker Sep 27 '17 at 16:39
  • 2
    \$\begingroup\$ The last version returns ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-2'] which is not ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '+', '2'] as advertized by the post. \$\endgroup\$ – Mathias Ettinger Sep 28 '17 at 8:51
  • \$\begingroup\$ @MathiasEttinger Yeah I forgot to change that, but they both evaluate to the same result \$\endgroup\$ – Blanker Sep 29 '17 at 1:23
1
\$\begingroup\$

The entire cleanUp method can be changed with almost 1 regular expression

Take the example:

import re
def clean(s):
    return re.split("([+\-\/*])", s.replace(" ", ""))

>>> print(clean("2+2 -4 * 1 / 3.344 - -2"))
>>> ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '', '-', '2']

This is almost the same as your output only difference is, it adds an empty '' when there are 2 operaters after eachother.

This can be fixed using a bit of list slicing, making the clean look like this:

import re
def clean(s):
    cleaned_expr = re.split("([+\-\/*])", s.replace(" ", ""))
    while '' in cleaned_expr:
        # Remove empty and concat operator to next number
        i = cleaned_expr.index('')
        cleaned_expr[i] = cleaned_expr[i+1] + cleaned_expr[i+2]
        del cleaned_expr[i+1:i+3]
    return cleaned_expr

>>> print(clean("2+2 -4 * 1 / 3.344 - -2"))
>>> ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-2']
>>> print(clean("2+2- -4 * 1 / 3.344 -2"))
>>> ['2', '+', '2', '-', '-4', '*', '1', '/', '3.344', '-', '2']

Some other minor PEP8 improvements are:

  1. naming of functions should be clean_up() instead of cleanUp
  2. Missing whitespaces around operators like =
  3. Missing whitespace around ,

Note

In retrospect and Thanks to @Mathias your calc() feels buggy, as an honest reviewer I would recommend it handles input like this ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-', '2'] when that is fixed I'm tempted to give another look at it, for now it just feels too buggy.

# The cleanest clean (but incorrect output for your calc)
import re
def clean(s):
    s = re.split("([+\-\/*])", s.replace(" ", ""))
    return [token for token in s if token]

>>> print(clean("2+2 -4 * 1 / 3.344 - -2"))
>>> ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-', '2']
\$\endgroup\$
  • \$\begingroup\$ That wil not work since, his calc() will break at this input ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-', '2'] \$\endgroup\$ – Ludisposed Sep 28 '17 at 9:37
  • \$\begingroup\$ In the end I think it would be best if it handles ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-', '2'] that output, since it is cleaner then ['2', '+', '2', '-', '4', '*', '1', '/', '3.344', '-', '-2'] \$\endgroup\$ – Ludisposed Sep 28 '17 at 9:49
  • \$\begingroup\$ Thinking back at it, neither version handle the case of 2---2 (should they? Python returns 0) as ['2', '-', '-', '-', '2'] is returned… So yes, better keep clean simple and fix calc instead… \$\endgroup\$ – Mathias Ettinger Sep 28 '17 at 9:58
  • \$\begingroup\$ I think to return 0 is a perfectly valid answer since 2 - + 2 = 0 it feels like I adapt clean() to work for calc() while perhaps it be better to fix calc() first \$\endgroup\$ – Ludisposed Sep 28 '17 at 10:04
  • 1
    \$\begingroup\$ Then return [token for token in cleaned_expr if token] should do instead of the while loop ;) \$\endgroup\$ – Mathias Ettinger Sep 28 '17 at 10:06
0
\$\begingroup\$

Regardless of the fact, that your code is not running properly as it returns

input formula error: line C in eval_expr
input formula error: line C in eval_expr

with the example input, I'd suggest simplifying your module to

from re import compile

ARITHMETIC_EXPRESSIONS = compile('[0-9]*( ){0,}([+-/*^]( ){0,}[0-9]*( ){0,})*')
LEADING_ZEROS = compile('^0+(?=.)')


def coerce_python_syntax(string):
    """Converts arithmetic expression to python syntax."""

    string = string.replace('^', '**')
    return LEADING_ZEROS.sub('', string)


def calc(string):
    """Calculates the arithmetic expression given by string."""
    if ARITHMETIC_EXPRESSIONS.fullmatch(string):
        return eval(coerce_python_syntax(string))

    raise ValueError('String is not a safe arithmetic expression.')


if __name__ == '__main__':
    print(calc("2+2 -4 * 1 / 3.344 - -2"))

since you are implementing basic arithmetic evaluation that python already has down cold.

\$\endgroup\$
  • \$\begingroup\$ Until something like: eval("os.system('rm -rf /')") comes along >;) \$\endgroup\$ – Ludisposed Sep 27 '17 at 11:49
  • \$\begingroup\$ Ahh I see .fullmatch now. I stand corrected, but still I hope you see, why eval is evil \$\endgroup\$ – Ludisposed Sep 27 '17 at 13:27
  • \$\begingroup\$ Without sanitizing the input, definitely. \$\endgroup\$ – Richard Neumann Sep 27 '17 at 13:27
  • \$\begingroup\$ The regex does miss a few, like operater ^ also try calc("01 + 10") or how about a CPU-burner attack? Stuff like 2**9999999999**9999999 \$\endgroup\$ – Ludisposed Sep 27 '17 at 13:54
  • \$\begingroup\$ Point is you'll end up with something quite long if you want to protect yourself for all, safe python \$\endgroup\$ – Ludisposed Sep 27 '17 at 13:56

Not the answer you're looking for? Browse other questions tagged or ask your own question.