Python Calculator using Postfix Expressions

Question

Improved upon my previous calculator program. This program has the following features:

1. Gives information using session prompts.

2. Supports all common operators.

3. Follows operator precedence.

4. Use of identifiers is supported.

5. Result of the previous expression can accessed by using the 'r' identifier.

6. Special commands:

   1. n: Stars a new session. Deletes all previous identifiers.
   2. q: Quits the program

The code describes the features in more detail.

Issues:

1. No useful information is given to the user if any error occurred.

Other Information:

1. Ditched the earlier use of eval() to evaluate expressions because of security reasons.

2. Avoids the endless function call chain used earlier that could hit the recursion limit albeit after a long while by using a loop.

My Previous calculator: A simple python3 calculator

Also uploaded the code on pastebin: https://pastebin.com/WW25hWn7

import sys
import math

usage_notes = """
1. Session prompts:
    1. n: New session
    2. c: Current session

2. Supported operators: +, -, *, /, ^, !

3. Precedence:
    1. Parenthesization
    2. Factorial
    3. Exponentiation
    4. Multiplication and Divison
    5. Addition and Subtraction

4. Use of identifiers is supported. Use commas to separate them:
    n: a=10,b=5
    c: a+b
    -> 15

5. Result of the previous expression can accessed by using the 'r'
identifier:
    n: 2+3
    -> 5
    c: r+10
    -> 15

6. Special commands:
    1. n: Stars a new session. Deletes all previous identifiers.
    2. q: Quits the program
"""

identifiers = {}


def start():
    # Creates a prompt dictionary to differentiate sessions.
    # Starts the main loop of the program.
    # Takes the input from the user and calls controller().

    prompts = {
        'n': 'n: ',
        'c': 'c: ',
    }

    prompt = prompts['n']
    while True:
        expr = input(prompt)
        if expr == 'q':
            break
        elif expr == 'n':
            prompt = prompts['n']
            identifiers.clear()
        else:
            res = controller(expr)

            if res == 'e':
                print('error: invalid expression\n')
            elif res == 'i':
                prompt = prompts['c']
            else:
                print('-> ' + identifiers['r'] + '\n')
                prompt = prompts['c']


def controller(expr):
    # Calls create_identifiers or passes the expr to get_postfix()
    # to be converted into a postfix expression list. And, calls
    # postfix_eval() for evaluation. All the Exceptions
    # are terminated, so the main loop keeps running. 

    try:
        if '=' in expr:
            return create_identifiers(expr)
        postfix_expr = get_postfix(expr)
        return postfix_eval(postfix_expr)
    except Exception:
        return 'e'


def create_identifiers(expr):
    # Identifiers are implemented as a global dictionary. First,
    # the string is split using ',' as a delimiter. The resulting
    # substring are separated using '='. First substring is assigned
    # as a key with second substring as the value.

    expr_list = expr.replace(' ', '').split(',')

    for stmt in expr_list:
        var, val = stmt.split('=')
        identifiers[var] = val
    return 'i'


def get_postfix(expr):
    # Converts infix expressions to postfix expressions to remove ambiguity.
    # Example: a+b*c -> abc*+

    # Remove all the spaces in the given expression.
    expr = expr.replace(' ', '')
    sep_str = ''

    # Insert spaces only around supported operators, so splitting 
    # can be done easily later.
    for a_char in expr:
        if a_char in '+-*/^!()':
            sep_str += ' %s ' % a_char
        else:
            sep_str += a_char

    # Use the default space as the delimiter and split the string.
    token_list = sep_str.split()

    # Only operators are pushed on to the op_stack, digits and identifiers
    # are appended to the postfix_list.
    op_stack = []
    postfix_list = []

    prec = {}
    prec['!'] = 5
    prec['^'] = 4
    prec['/'] = 3
    prec['*'] = 3
    prec['+'] = 2
    prec['-'] = 2
    prec['('] = 1

    # The current operator's precedence in the loop is compared with the
    # operators in the stack. If it's higher, it's pushed on the stack.

    # If it less than or equal, the operators are popped until the
    # precedence of the operator at the top is less than the 
    # current operators'.

    # When parentheses are used, ')' forces all the operators above '('
    # to be popped.

    # Whenever an operator is popped it's appended to the postfix_list.
    for token in token_list:
        if isnum(token) or token.isalpha():
            postfix_list.append(token)
        elif token == '(':
            op_stack.append(token)
        elif token == ')':
            top_token = op_stack.pop()
            while top_token != '(':
                postfix_list.append(top_token)
                top_token = op_stack.pop()
        else:
            while op_stack != [] and \
                (prec[op_stack[-1]] >= prec[token]):
                postfix_list.append(op_stack.pop())
            op_stack.append(token)

    while op_stack != []:
        postfix_list.append(op_stack.pop())

    return postfix_list


def postfix_eval(postfix_list):
    # Similar stack based approach is used here for evaluation. If a
    # identifier or digit is found, push it on the operand_stack. If
    # an operator is found, use it on the last two operands or the last
    # in case of '!', and append the result on the stack.

    operand_stack = []

    for val in postfix_list:
        if isnum(val):
            operand_stack.append(float(val))
        elif val.isalpha():
            val = identifiers[val]
            operand_stack.append(float(val))
        elif val in '+-*/^!':

            if val != '!':
                op2 = operand_stack.pop()
                op1 = operand_stack.pop()
                res = calc(op1, val, op2)
                operand_stack.append(res)
            else:
                op = operand_stack.pop()
                res = math.factorial(op)
                operand_stack.append(res)

    res = operand_stack[-1]
    int_res = int(res)
    if int_res == res:
        res = int_res

    identifiers['r'] = str(res)


def isnum(val):
    # Used as a helper function to check if the argument is a number.
    try:
        float(val)
        return True
    except Exception:
        return False


def calc(op1, op, op2):
    # Performs the operation on the operands and returns the result.
    if op == '+':
        return op1 + op2
    elif op == '-':
        return op1 - op2
    elif op == '*':
        return op1 * op2
    elif op == '/':
        return op1 / op2
    elif op == '^':
        return op1 ** op2

if sys.argv[-1] == 'n':
    print(usage_notes)

start()

Sample output:

n: 1+5
-> 6

c: r
-> 6

c: r*10+50*(3-1)-100
-> 60

c: r
-> 60

c: a=50,b=10
c: r+a+b
-> 120

c: n
n: 2^3+5!
-> 128

c: 1 +- 2
error: invalid expression

c: q

I am looking for ways to make the code more readable, minimize redundancy, improving the overall design, performance improvements, etc. Any help would be appreciated!

Answer 1

First, the most glaring thing I see is your use of catch-all exception handlers:

def controller(expr):
    . . .
    # . . . All the Exceptions
    # are terminated, so the main loop keeps running. 
    . . .
    except Exception:
        return 'e'

. . .

res = controller(expr)

if res == 'e':
    print('error: invalid expression\n')

I have to strongly caution against this practice. You're treating every exception that may possibly come up as though it's caused by a runtime error stemming from bad user input. This is a very dangerous assumption, and will bite you eventually.

In this case, why is it such a big deal? Inside the try, you're calling three different functions, and all of those functions call several other functions. Lets say in the future you decide to modify one of the many functions that are called as a result of calling controller, and lets say you accidentally introduce a bug. In a normal workflow, you'd try to run controller as a test, it would fail horribly because you wrote something incorrect somewhere, and you can use the resulting stack trace to diagnose what caused the problem.

With how you have it here, you would run it... and it would complain about an invalid expression being entered. How much information does that give you about what happened? Throwing away the clues that help you debug a problem will only make your life more difficult once you start writing larger programs and start encountering hard-to-reproduce bugs.

The better way to approach this is to specify the exact exception that you expect to catch instead of using a catch-all Exception. If you expect a ValueError to be thrown if the expression was poorly formed, catch that. That way any other exception that may be raised will still come through. A broken program crashing is a good thing. Let it fail so you can fix it.

The same problem, but to a lesser extent can be seen in isnum (which I'd rename to at least is_num):

def is_num(val):
    # Used as a helper function to check if the argument is a number.
    try:
        float(val)
        return True
    except Exception:
        return False

float seems to only throw two types of exceptions; and only one of them seems relevant here. Change the catch to except ValueError. This isn't a big deal right now since only the call to float is inside the try, but if you add anything later you're opening yourself up to silent failings.

In this code, catch-all exceptions won't be the end of the world. They are a bad habit to get into though, and don't encourage a safe mindset. Be aware of what exceptions the code you're using can throw and react accordingly. Catching everything is just a band-aid.

I'd also space your code out a bit. I personally like empty lines after "bodies" of code, like the bodies of a if...else, or a try...except:

def is_num(val):
    try:
        float(val)
        return True

    except Exception:
        return False

def controller(expr):
    try:
        if '=' in expr:
            return create_identifiers(expr)

        postfix_expr = get_postfix(expr)
        return postfix_eval(postfix_expr)

    except Exception:
        return 'e'

I like giving discrete parts some breathing room. I find it helps readability.

---

prec = {}
prec['!'] = 5
prec['^'] = 4
prec['/'] = 3
prec['*'] = 3
prec['+'] = 2
prec['-'] = 2
prec['('] = 1

This could be written as a literal, and I think it would be neater as a result:

prec = {'!': 5,
        '^': 4,
        '/': 3,
        '*': 3,
        '+': 2,
        '-': 2,
        '(': 1}

---

Your use of global identifiers isn't ideal. I'd prefer to pass a state around using an explicit parameter to any functions that require access to identifiers. That would make testing functions that use identifiers much easier. With how you have it now, whenever you want to test a function like postfix_eval that uses identifiers, you need to make sure to do identifiers = some_test_state before your call. If it were a parameter, its dependencies would be explicit, and it wouldn't require accessing a global mutable state.

---

A lot of your functions start with some comments that describe the action of the function:

def calc(op1, op, op2):
    # Performs the operation on the operands and returns the result.

This is a good habit to get in. Python has a standardized convention though to handle comments intended for the end-user of the function: docstrings. Right after the function "signature", have a (triple) String literal instead of using # line comments. IDEs will grab this information and allow it to be accessed easier by callers.

def calc(op1, op, op2):
    """ Performs the operation on the operands and returns the result. """