Elena Volkova #12
Elena Volkova #12
Conversation
Signed-off-by: Elena Volkova <[email protected]>
Realization construction in the degree and division
Issues: * Double operations of different priorities were ignored * Ignored redundunt closed brackets
|
В требованиях было отмечено следующее:
Не смотря на то, что минимальное изменение проверяющего скрипта и конфигурационного файла для CI делает возможным проверку работы без Есть много статей, в которых подробно описан процесс создания Можно найти хорошее описание самих атрибутов в функции |
| MATH_ACTIONS = ("+", "-", "*", "/", "%", "^",) | ||
| COMPARISON_OPERATIONS = (">", "<", "=", "!",) | ||
|
|
||
| def __init__(self, expression, func=None): |
There was a problem hiding this comment.
В инициализаторе класса исполняется большое количество логики. Возможно стоит разбить эту логику на набор функций и вызывать эти функции в __init__ , либо каким-то другим образом уменьшить размер этой функции.
| # Look for commas in expression | ||
| start_index = 0 | ||
| multivalue_items = [] | ||
| for i, c in enumerate(expression): |
There was a problem hiding this comment.
Я думаю, лучше избегать названий переменных из одного символа.
вместо i лучше использовать index, вместо v лучше использовать value и т.д. и т.п.
Много места это не отнимет, но код будет читаться почти как английский язык и другим людям будет проще понять идеи, которые реализовывал автор.
| String representation of the class | ||
| :return: string representation of the class | ||
| """ | ||
| result = [] |
There was a problem hiding this comment.
Может быть заменено на генератор списка. Красивие выглядит, быстрее работает.
result = [str(item) for item in self._expression]| return boolean_value | ||
| return boolean_value | ||
|
|
||
| # Calculate mathematical expression |
There was a problem hiding this comment.
Этот комментарий можно опустить, так как он в точности повторяет название функции.
Подобные ситуации есть и в других местах.
| return value | ||
|
|
||
| # Calculate value expression | ||
| def value(self): |
There was a problem hiding this comment.
Я бы переименовал название этой функции таким образом, чтобы в ее названии было отображено какое-то действие (так как функция это и есть действие, а не объект, который описывается существительным). Возможно комментарий на строчку выше будет хорошим кандидатом для названия этой функции.
| if last_operation: | ||
| if last_operation in ("+", "-",): | ||
| if v in ("+", "-"): | ||
| if last_operation == v: |
There was a problem hiding this comment.
Для экономии места в этом и некоторых других местах можно использовать тернарный оператор.
last_operation = "+" if last_operation == v else "-"| try: | ||
| expression = Element(expression=args.EXPRESSION) | ||
| print(expression.value()) | ||
| except BaseExpressionException as exc: |
There was a problem hiding this comment.
Что если в приложении произойдет какая-то ошибка, которую мы не ожидаем (все люди ошибаются, код не может быть идеальным)? например где-то произойдет неожиданный для нас ValueError.
В таком случае исключение будет не обработано и стек вызовов будет распечатан пользователю в консоль.
Я думаю, что такую информацию пользователь не должен видеть.
Возможно в данном случае будет лучше отлаливать Exception, хотя мое мнение может быть оспорено.
VolkovaElena
force-pushed
the
develop
branch
from
4a2926e to
cabe3c5
Compare
3 participants
No description provided.