patternpythonModerate
Newborn pythonic calculator
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:
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
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:
I'm wondering if you have a particular reason for not writing simply:
The same goes for all the
Compiling regexes
I'm a bit surprised by this:
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:
Using
(This might be subjective.)
Another alternative is to enclose the complex expression within parens:
But this also isn't too good:
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:
The Pythonic way is to simply:
Returning boolean values directly
I'm a bit surprised by this:
if type(self) != type(other):
return False
return TrueI'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 Truereturn 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.