Metelskiy Ivan #14
Metelskiy Ivan #14
Conversation
final_task/constants.py
Outdated
| HIGHEST_PRIORITY = 4 | ||
|
|
||
| RE_MAIN_PARSE_ARG = re.compile(r'\d+[.]\d+|\d+|[+,\-*^]|[/=!<>]+|\w+|[()]') | ||
| # |
There was a problem hiding this comment.
Для чего тут пустые комментарии?
final_task/logic.py
Outdated
| import pycodestyle | ||
| from typing import Any | ||
|
|
||
| pycodestyle.maximum_line_length = 120 |
There was a problem hiding this comment.
в данном случае лучше передавать такие настройки напрямую утилите pycodestyle, без подстройки этой конфигурации в самом калькуляторе
final_task/logic.py
Outdated
| for import_elem in usr_imports: | ||
| import_objects[import_elem] = importlib.import_module(import_elem) | ||
|
|
||
| globals().update(import_objects) |
There was a problem hiding this comment.
Опасный подход с хранением импортированных объектов напрямую в глобальной секции. Намного лучше будет хранить эти объекты в отдельном словаре (либо что-то другое, но точно не стоит хранит это в globals)
final_task/logic.py
Outdated
| import_objects = dict() | ||
|
|
||
| for import_elem in usr_imports: | ||
| import_objects[import_elem] = importlib.import_module(import_elem) |
There was a problem hiding this comment.
import_objects = {import_elem: importlib.import_module(import_elem) for import_elem in usr_imports}
final_task/logic.py
Outdated
| :param methods: dictionary which contains objects of methods of imports | ||
| :return: number by its type | ||
| """ | ||
| if not type(item) is str: |
There was a problem hiding this comment.
if not isinstance(item, str)| globals().update(import_objects) | ||
|
|
||
|
|
||
| def _get_item_by_type(item: str, methods: dict) -> Any: |
There was a problem hiding this comment.
Одна точка выхода из функции обычно лучше, чем несколько. В данном случае можно сделать только один return в функции без особых усилий и читаемость кода не пострадает.
final_task/logic.py
Outdated
| :param unit: name of unit as string | ||
| :return: dictionary which contains name of attribute as key and it object as value | ||
| """ | ||
| attrs = tuple([i for i in dir(globals()[unit])]) |
There was a problem hiding this comment.
В данном случае сначала создается список, а потом приводится к кортежу :) Созание списка в этом случае абсолютно бесполезно.
attrs = tuple(i for i in dir(globals()[unit]))И снова, советую совсем избавиться от globals
final_task/logic.py
Outdated
| if ex_str.count('(') != ex_str.count(')'): | ||
| raise Exception('brackets are not balanced') | ||
|
|
||
| while re.compile(r'[+][-]|[-][+]').findall(ex_str): |
There was a problem hiding this comment.
Регулярные выражения лучше выносить с отдельные переменные с говорящим названием.
final_task/logic.py
Outdated
| :param expression: mathematical operator | ||
| :return: priority of given operator as integer | ||
| """ | ||
| if expression == constants.lowest_priority_operator: |
There was a problem hiding this comment.
Как вариант, хранение обозначения оператора и его приоритет можно связать в какую-то общую структуру.
либо можно сделать удобный мапинг в виде словаря, который позволит избавиться от цепочки конструкций elif
final_task/logic.py
Outdated
| return packed_expression | ||
|
|
||
|
|
||
| def _priority(expression: str) -> int: |
final_task/logic.py
Outdated
| return -1 | ||
|
|
||
|
|
||
| def polish_notation(expression_list: list, methods: dict) -> list: |
There was a problem hiding this comment.
Название функции/метода должно обозначать действие
There was a problem hiding this comment.
Я насчитал в этой функции 14 операторов условий, что на самом деле очень много. Если есть возможность "без боли" разбить эту функцию на несколько маленьких, то это бы значительно упростило понимание кода.
final_task/mainclass.py
Outdated
|
|
||
| class calculator: | ||
|
|
||
| def __init__(self, user_imports=[]): |
There was a problem hiding this comment.
Не стоит использовать список как значение по умолчанию для аргумента функции:)
Дефолтные значения создаются единожды во время создания объекта функции, а не при каждом вызове функции, что может привести к неожиданным багам))
final_task/mainclass.py
Outdated
| polish_list = logic.polish_notation(normal_list, self._methods) | ||
| return logic.ex_calc(polish_list, self._methods) | ||
| except Exception as ex: | ||
| print('Error: {}\n'.format(ex)) |
There was a problem hiding this comment.
На CI проверяется, что ошибка начинается с ERROR: (большимим буквами)
final_task/mainclass.py
Outdated
| pycodestyle.maximum_line_length = 120 | ||
|
|
||
|
|
||
| class calculator: |
There was a problem hiding this comment.
На самом деле это бесполезный класс, всю его функциональность можно оформить в функцию
final_task/logic.py
Outdated
| found_func = False | ||
| params_list.append(one_param_list) | ||
| one_param_list = [] | ||
| for list in params_list: |
There was a problem hiding this comment.
list уже есть в built-in
Нужно дать другое имя переменной, чтобы не было перекрытия
|
|
||
|
|
||
| def parse_funcs_params(ex_list: list, methods: dict) -> list: | ||
| """ |
There was a problem hiding this comment.
Можешь взглянуть на библиотеку typing. Она позволяет более явно указывать содержимое коллекций.
No description provided.