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

Newborn pythonic calculator

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
newbornpythoniccalculator

Problem

Let me start off by saying that I have several years of experience in Java, but now I need to learn Python, so I decided to make a calculator as it also is a community challenge.

Please review the code looking for beginner mistakes, though I do intend the code to look professional. This is my first real program made in Python and two days ago I knew nothing about Python.

My calculator currently has the following abilities:

  • It evaluates expressions, so you cannot interact with it.



  • The supported operators are +, -, * and /.



  • Functions are not supported.



  • It uses the Shunting-yard algorithm.



  • It uses Reverse Polish Notation.



calculator/tokens.py

```
from decimal import Decimal

__author__ = 'Frank van Heeswijk'

class Token:
pass

class ValueToken(Token):
def __init__(self, value: Decimal):
self.value = value

def __repr__(self):
return "VT(" + str(self.value) + ")"

def __hash__(self):
return hash(self.value)

def __eq__(self, other):
if type(self) != type(other):
return False
return self.value == other.value

def __ne__(self, other):
return not self == other

class OperatorToken(Token):
def __init__(self, operator: str):
self.operator = operator

def __repr__(self):
return "OT(" + self.operator + ")"

def __hash__(self):
return hash(str)

def __eq__(self, other):
if type(self) != type(other):
return False
return self.operator == other.operator

def __ne__(self, other):
return not self == other

class LeftParenthesesToken(Token):
def __repr__(self):
return "LPT"

def __hash__(self):
return 0

def __eq__(self, other):
if type(self) != type(other):
return False
return True

def __ne__(self, other):
return not self == other

class RightParenthesesToken(Token):
def __repr__(self):
return "RPT"

def __hash

Solution

This looks pretty nice! I have only a few minor nitpicks and practical tips for you.

Returning boolean values directly

I'm a bit surprised by this:

if type(self) != type(other):
        return False
    return True


I'm wondering if you have a particular reason for not writing simply:

return type(self) == type(other)


The same goes for all the __eq__ methods that can be simplified to a single return statement.

Compiling regexes

I'm a bit surprised by this:

def tokenize(self, expression: str) -> list:
    # ...

    value_regex = re.compile(r"\d+(\.\d+)?")
    operator_regex = re.compile(r"[^\d\.\(\)]")
    left_parentheses_regex = re.compile(r"\(")
    right_parentheses_regex = re.compile(r"\)")


Compiling the regexes every time this method gets called?
Usually I put pre-compiled regexes at the top of the file as global constants.
But looking at the docs,
I'm wondering if compiling a few regexes is necessary at all,
as it seems there is a built-in caching mechanism.

Taming complex conditions

This ain't pretty:

if isinstance(token, OperatorToken) and token.operator == '-':
    if index == 0\
    or isinstance(tokens[index - 1], LeftParenthesesToken)\
    or isinstance(tokens[index - 1], OperatorToken):
        tokens[index] = OperatorToken('u-')


Using \ to break long lines tends to be a bit ugly.
(This might be subjective.)
Another alternative is to enclose the complex expression within parens:

if (index == 0
        or isinstance(tokens[index - 1], LeftParenthesesToken)
        or isinstance(tokens[index - 1], OperatorToken)):
        tokens[index] = OperatorToken('u-')


But this also isn't too good:

  • PEP8 complains: E129 visually indented line with same indent as next logical line



  • If I increase the indent of tokens[index] = ..., same complaint (to be honest, I don't really understand that)



  • I can play with the indents further to get something that passes PEP8 but doesn't actually look reasonable



The bottom line is, the best solution for a complex boolean expression is to extract to a helper function.
The function can be defined anywhere,
even right before you use it.
It will the added benefit of having a name.

Or maybe none of this is worth the trouble and your original is best.
Just some food for thought.

The Pythonic way of checking empty things

Instead of:

while len(stack) > 0:
    pop_token = stack.pop()


The Pythonic way is to simply:

while stack:
    pop_token = stack.pop()

Code Snippets

if type(self) != type(other):
        return False
    return True
return type(self) == type(other)
def tokenize(self, expression: str) -> list:
    # ...

    value_regex = re.compile(r"\d+(\.\d+)?")
    operator_regex = re.compile(r"[^\d\.\(\)]")
    left_parentheses_regex = re.compile(r"\(")
    right_parentheses_regex = re.compile(r"\)")
if isinstance(token, OperatorToken) and token.operator == '-':
    if index == 0\
    or isinstance(tokens[index - 1], LeftParenthesesToken)\
    or isinstance(tokens[index - 1], OperatorToken):
        tokens[index] = OperatorToken('u-')
if (index == 0
        or isinstance(tokens[index - 1], LeftParenthesesToken)
        or isinstance(tokens[index - 1], OperatorToken)):
        tokens[index] = OperatorToken('u-')

Context

StackExchange Code Review Q#88056, answer score: 15

Revisions (0)

No revisions yet.