Evaluating arithmetic expressions

I have written some simple code for evaluating expressions. However, I am not sure how well I am following conventions (this is my first time trying to).

Specs for problem:

• All binary operators (+, -, *, /), no unary - or +

• Uses PEMDAS

My code is:

```
import re
import time
DIGITS = '0123456789'
OPS = '+-*/^'
OP_FUNCS = {
'+':lambda x, y:x + y,
'-':lambda x, y:x - y,
'':lambda x, y:x y,
'/':lambda x, y:x / y,
'^':lambda x, y:x ** y,
}
ORDER_OF_OPERATIONS = [
['^'],
['*', '/'],
['+', '-'],
]
VALID_PAIRS = [
('NUM', 'OP'),
('OP', 'NUM'),
('OP', 'OPAREN'),
('CPAREN', 'OP'),
('OPAREN', 'NUM'),
('NUM', 'CPAREN'),
('OPAREN', 'OPAREN'),
('CPAREN', 'CPAREN'),
]
NUM_MATCH = re.compile(
'(?:[1-9][0-9]*|0)'
'(?:[.][0-9]+)?'
)
class Token(): #This is not really useful but tuples could be less clear
def __init__(self, type_, info=None):
self.type = type_
self.info = info
# def __str__(self):
# return '{}:{}'.format(self.type, self.info)
PLACEHOLDER = Token('PLACEHOLDER')
def tokenize(expr):
tokens = []
index = 0
while index<len(expr):
curr_and_after = expr[index:]
is_num = NUM_MATCH.match(curr_and_after)
if expr[index] in OPS:
tokens.append(Token('OP', expr[index]))
elif is_num:
num = is_num.group(0)
tokens.append(Token('NUM', float(num)))
length = len(num)
index += length-1
elif expr[index] == '(':
tokens.append(Token('OPAREN'))
elif expr[index] == ')':
tokens.append(Token('CPAREN'))
elif expr[index] == ' ':
pass
else:
raise SyntaxError('Invalid character')
index += 1
return tokens
def is_valid(tokens):
if tokens == []:
return False

#This sections tests if parentheses are correctly nested
nesting = 0
for token in tokens:
if token.type == 'OPAREN':

Spacing

I'd like to see some more blank lines. They help separate code into blocks. Without a blank line here and there, the code looks like a wall of text. As someone who answers questions on Stack Overflow, I can tell you a wall of text is hard to read; I usually just skip such questions. Of course, someone reading your source code may be forced not to skip it, but you want to make it easy.

Documentation

Your code is well-organized and clear. With the comments in addition, I can almost forgive the lack of doc strings, yet doc strings are used for more than just the people who read the source code itself. Bots such as pydoc read it too for generating documentation. The comments are no replacement for that. Each function's purpose should be clear without the doc strings, but documentation takes out the guesswork.

operator

I'm glad you're using a dictionary for OP_FUNCS, but you don't need to define your own functions. Python has a built-in module for that sort of thing: operator. You can use it like this:

import operator

OP_FUNCS = {
    '+': operator.add,
    '-': operator.sub,
    '*': operator.mul,
    '/': operator.truediv,
    '^': operator.pow,
}

If you take a look through that module's documentation, maybe you'll be inspired to add more operators. It's very easy when the functions are already defined for you; you just need to figure out a symbol for it and its place in the order of operations.

Generators

You should use generators more often. They are more memory-efficient than lists because each value is generated on the fly instead of needing to remember them all at once.

This is especially useful in has_op(), for example. All you need to do is remove the opening and closing brackets. That changes to using a generator expression instead of a list comprehension. Let's say the first item in tokens is a match. If you use a generator expression, any() will return True right away, and none of the other tokens is even checked. When you use a list comprehension, any() doesn't start to do anything until all tokens have been processed.

It is also useful in eval_tokens when you are using has_op. In short, it is rare indeed if a list comprehension is to be preferred over a generator expression when using any().

You have other functions that might work nicely as generator functions, except that the functions using them all require them to be lists, so that would mean that they would need to use the list function. It might still be a good idea, but it isn't a clear advantage.

Miscellaneous

num = is_num.group(0)
...
length = len(num)
index += length-1

The match object has methods for finding the edges of a match. Since you're using .match(), the match is guaranteed to begin at the start of the string, so you can use simply .end():

index += num.end() - 1

if tokens == []:
    return False

Instead of creating a blank list to compare to, use the list's boolean value:

if not tokens:
    return False

This will now work even if tokens is a blank tuple, for example.

possible_valid_pairs = []
for valid_pair in VALID_PAIRS:
    possible_valid_pairs.append((curr_kind, next_kind) == valid_pair)
if not any(possible_valid_pairs):
    return False

Instead of creating a list of comparison booleans, use the built-in keyword:

if (curr_kind, next_kind) not in VALID_PAIRS:
    return False

newTokens = []

This is the only place in your code that I see a variable using lowerCamelCase. I suppose you had a bad day and you were caught in a moment of weakness. I'll forgive you this time; just make sure it doesn't happen again.

Conclusion

Despite these criticisms, this is still a very well-written script. It's logical, extendable, organized, and easy to read. There are comments in many places, and most of them are useful explanations that further enhance the readability. This, in short, is a script to be proud of.

StackExchange Code Review Q#160524, answer score: 8