Anton Charnichenka #1
Conversation
Signed-off-by: Aliaksei Buziuma <[email protected]>
AntonCharnichenka
force-pushed
the
work
branch
from
7c56024 to
03d82b6
Compare
PythonAndGoCommunity
force-pushed
the
master
branch
2 times, most recently
from
b5a4c19 to
17385ed
Compare
PythonAndGoCommunity
left a comment
PythonAndGoCommunity
left a comment
There was a problem hiding this comment.
Full feedback will be provided later by email.
| @@ -0,0 +1,266 @@ | |||
| """This module contains unit tests for methods and functions from all pycalc modules""" | |||
|
|
|||
| # import | |||
There was a problem hiding this comment.
Такие комментарии повторяют информацию, которую нам может дать код, к которому эти комментарии прикреплены. Эти комментарии могут быть опущены либо оформлены как docstring для функций или классов.
Похожие комментарии есть в этом файле в строках 13, 17, 120, 154, 183 и в других файлах.
| rpn_tokens = ['2', 'sqrt', '3', '/', '3.14', '*', 'tan'] | ||
| rpncalculator = RPNcalculator(rpn_tokens, pycalclib) | ||
| result, error_msg = rpncalculator.evaluate() | ||
| self.assertEqual(result, 11.009005500434151) |
There was a problem hiding this comment.
Возможно есть смысл заменить магическую константу в этой строке на выражение на языке Python, чтобы было четко видно, что эта функциональность должна повторять функциональность языка Python. Также в таких случаях можно использовать метод для проверки приблизительного равенства чисел assertAlmostEqual https://docs.python.org/3/library/unittest.html#unittest.TestCase.assertAlmostEqual
| def main(): | ||
| """Calculates user math expression""" | ||
| parser = create_parser() | ||
| namespace = parser.parse_args(sys.argv[1:]) |
There was a problem hiding this comment.
В данном случае необязательно явно отправлять sys.argv[1:]
можно опустить отправку этого аргумента в эту функцию
| tokens[index] = tokens[index][1:] # remove '-' sign | ||
| tokens.insert(index, '-1') | ||
| tokens.insert(index+1, '*') | ||
| index += 3 |
There was a problem hiding this comment.
Магическое число. возможно есть смысл перенести число 3 в отдельную переменную или добавить поясняющий комментарий, почему прибавляется именно тройка
| from .pycalclib import Pycalclib | ||
|
|
||
|
|
||
| class Constsreplacer: |
There was a problem hiding this comment.
Название класса -- ConstsReplacer
Название модуля -- consts_replacer
https://www.python.org/dev/peps/pep-0008/#package-and-module-names
| @@ -0,0 +1,31 @@ | |||
| """This module contains a class that allows to replace constants with their numeric equivalents""" | |||
There was a problem hiding this comment.
Использование класса, а темболее модуля для инкапсюляции подобного функционала выглядит очень избыточно. Можно было бы обойтись обычной функцией. stop writing classes!
| """Initialize pycalclib""" | ||
| self.user_module = user_module | ||
| # r_strings that are used to find operators / functions / etc | ||
| self.r_one_sign_operators = [r'^\+', r'^-', r'^\*', r'^/', r'^\^', r'^%'] |
There was a problem hiding this comment.
Для хранения данных, которые не будут никогда изменяться, имеет смысл использовать кортеж вместо списка. Таким образом мы явно указываем, что это неизменяемые данные, и используем меньше памяти.
В целом содержимое данного модуля является очень избыточным. Например functions мог бы быть сгенерирован при помощи
import math
functions = tuple(x for x in dir(math) if callable(x) and not x.startswith('_'))Остальное по аналогии.
| '==': operator.eq, '!=': operator.ne} | ||
| # functions actions | ||
| # first element in a tuple associated with each key is a number of parameters for corresponding function | ||
| self.functions_dict = {'acos': (1, math.acos), 'acosh': (1, math.acosh), 'asin': (1, math.asin), |
There was a problem hiding this comment.
В данном случае для хранения информации о функции и самой функции лучше подойдет namedtuple вместо обычного кортежа.
| 'trunc': (1, math.trunc), 'abs': (1, lambda x: abs(x)), | ||
| 'round': (1, lambda x: round(x)), '-abs': (1, lambda x: -abs(x))} | ||
|
|
||
| if self.user_module != '': |
There was a problem hiding this comment.
if self.user_module:
...| else: | ||
| return True | ||
|
|
||
| def convert2rpn(self): |
There was a problem hiding this comment.
Лучше назвать функцию convert_to_rpn
| or (self.precedence[self.operators_stack[-1]] > self.precedence[current_token]) | ||
| or (self.precedence[self.operators_stack[-1]] == self.precedence[current_token] | ||
| and self.is_left_associative(self.operators_stack[-1]))) | ||
| and (self.operators_stack[-1] != "(")): |
There was a problem hiding this comment.
Лучше использовать один тип кавычек на один файл, либо “ либо ‘. Отличный вид кавычек есть смысл использовать, когда в строке встречается много кавычек, которые нужно экранировать. В других случаях лучше придерживаться одного стиля.
| elif current_token == ',': | ||
| self.error_msg = "ERROR: incorrect usage of ','" | ||
| break | ||
| elif len(self.operators_stack) == 0 and len(self.output_queue) != 0: |
There was a problem hiding this comment.
elif not self.operators_stack and self.output_queue:
3 participants
No description provided.