[WIP] Oleg Liasota #13
Conversation
final_task/pycalc.py
Outdated
| '^': 3, | ||
| '(': 4, ')': 4} | ||
|
|
||
| func_math = math.__dict__ |
There was a problem hiding this comment.
в атрибуте math.__dict__ лежат не только функции, но и в целом все атрибуты, которые есть в этом модуле. Либо каким-то образом нужно достать из этого словаря именно все функции, либо изменить название константы.
There was a problem hiding this comment.
Все константы должны быть напианы большимы буквами и разделены нижним подчеркиванием pep8
final_task/pycalc.py
Outdated
|
|
||
|
|
||
| def translate_operations(operation, x, y): | ||
| calculate = { |
There was a problem hiding this comment.
Необычный подход.
В данном случае словарь с маппингом функций будет каждый раз создаваться заново при каждом вызове функции translate_operations. Этот словарь может быть определен например в глобальной переменной, но точно не должен создаваться каждый раз в этой функции
There was a problem hiding this comment.
Посмотри в сторону модуля operator. Возможно он подойдет лучше для тебя, чем создание новых лямбда-функций
final_task/pycalc.py
Outdated
| result = calculate_res_exception(res_expression) | ||
| print(result) | ||
|
|
||
| main() |
There was a problem hiding this comment.
Не нужно вызывать функцию main прямо в глобальной области.
Все, что определено в глобальной области, выполняется во время импорта модуля. Это значит, что если твой модуль будет импортироваться из другого файла, то будет вызвана функция main, хотя например тебе нужно было только сделать импорт какой-то переменной.
Вызов main функции лучше оформлять вот так (в таком случае main будет вызван только когда этот файл является точкой входа в программу)
if __name__ == '__main__':
main()
final_task/pycalc.py
Outdated
| elif operations[-1].isalnum(): | ||
| res_expression.append(operations[-2]) | ||
| res_expression.append(operations[-1]) | ||
| del operations[-1] |
There was a problem hiding this comment.
вместо del лучше использовать метод .pop. Он как раз возвращает нужный элемент и удаляет его из коллекции
final_task/pycalc.py
Outdated
| operations[i-2] += 1 | ||
|
|
||
|
|
||
| def translate_reverse_exception(expression, operations, res_expression): |
There was a problem hiding this comment.
Многие функции работают по принципу изменения аргументов, которые пришли функции. Большинство функций не имеют возвращаемых значений. Я бы немного изменил подход и пытался по возможности реализовывать так называемые чистые функции.
final_task/pycalc.py
Outdated
| elif item == ',': | ||
| increase_number_parameters(operations) | ||
| unloading_operations(res_expression, operations) | ||
| elif item == 'e': |
There was a problem hiding this comment.
Дискриминация к константе tau :)
Я советую изменить подход к константам и сделать что-то похожее, как сделано с функциями. Не нужно завязываться на имена переменных в данном случае. Можно сделать такой же маппинг переменных, как с функциями. Ключ -- это название константы, значение -- это значение константы.
final_task/calculator/calc.py
Outdated
| DICT_MATH['abs'] = abs | ||
|
|
||
|
|
||
| def calculate_function(res_expression, dicts_modules, i): |
There was a problem hiding this comment.
Все еще есть немало функций, которые работают по принципу изменения входящих аргументов. ПО возможности от такого надо избавляться. Лучше, чтобы функции не изменяли входные аргументы, а возвращали какие-то новые значения.
Если же очень надо изменять какое-то состояние, я бы лучше вместо изменяния аргументов в данной ситуации использовал класс и изменял атрибуты объекта (тут и в других функциях)
Изменяние аргументов может приводить к очень странному и неочевидному поведению.
final_task/calculator/validation.py
Outdated
|
|
||
|
|
||
| def parse_added_modules(): | ||
| modules = parse_command_line()[1] |
There was a problem hiding this comment.
- Если рассматривать конкретно эту ситуацию, то немного красивее будет написать вот так:
_, modules = parse_command_line()- Если рассматривать в целом, то лучше из функции возвращать объект с аргументами командной строки
parser.parse_args(), а потом уже забирать из объекта то, что надо
если не ошибаюсь, есть в аргпарсе возможность задавать имя этих атрибутов для объекта
https://docs.python.org/3/library/argparse.html#dest
final_task/calculator/validation.py
Outdated
| for item in modules: | ||
| dicts_modules.append(__import__(item).__dict__) | ||
| except ModuleNotFoundError: | ||
| return 'ERROR: Module Not Found' |
There was a problem hiding this comment.
В этом месте и в некоторых других местах прослеживается такая проблема.
Функция имеет несколько точек выхода + в разных точках возвращаются абсолютно разные объекты с точки зрения типа и с точки зрения логики
в данном случае, если все ок, то функция вернет список из словарей с атрибутами модулей, а если произошла ошибка, то вернется строчка с ошибкой
Разные типы, разное наполнение объектов и в целом это разные объекты, которые никак не связаны
такой подход усложняет использование этой фунцкции, нужно всегда проверять возвращаемое значение функции и только после этой проверки можно что-то делать, хотя этих проверок можно легко избежать
- для ошибок я все такие советуюю использовать исключения. В данном случае произошла ошибка, после которой программа должна завершиться. Поэтому в данном случае можно сгенерировать новое исключение, которое должно быть обработано на самом высоком уровне.
- очень желательно, чтобы у функций был один return. А если все таки необходимо сделать два return, то они должны возвращать как минимум схожие по смыслу объекты
final_task/calculator/validation.py
Outdated
| return False | ||
|
|
||
|
|
||
| def del_spaces(expression): |
There was a problem hiding this comment.
Я бы не реализовывал для этой задачи отдельную функцию, а использовал что-то из стандартной библиотеки
Например вот это ''.join(sentence.split()) либо как-то по-другому)
final_task/calculator/calc.py
Outdated
| break | ||
| i += 1 | ||
| else: | ||
| if type(self.res_expression[0]) is not float and type(self.res_expression[0]) is not bool and \ |
There was a problem hiding this comment.
Для таких проверок можно использовать функцию isinstance
| DICT_MATH['abs'] = abs | ||
|
|
||
|
|
||
| class CALCULATOR: |
There was a problem hiding this comment.
class Calculator:
...| else: | ||
| return self.error | ||
|
|
||
| def calculate_function(self, dicts_modules, i): |
There was a problem hiding this comment.
В данной функции нет документации. Есть аргумент i. Получается нет никакой возможности понять, что это за i, кроме как разобраться в большом куске кода. односимвольные переменные точно никогда не стоит использовать как аргументы функции.
| self.res_expression.append(self.operations.pop(-2)) | ||
| self.res_expression.append(self.operations.pop()) | ||
|
|
||
| def search_func(self): |
There was a problem hiding this comment.
Функция называется search_func, но возвращает индекс.
Лучше подойдет get_func_index или что-то в этом духе
| i = i - 1 | ||
| return i | ||
|
|
||
| def write_func(self, i): |
There was a problem hiding this comment.
функция называется write_func, но она никуда и ничего не пишет
|
Сообщения коммитов в гите должны нести какой-то смысл изменения, которое было сделано в этом коммите. Сообщение |
|
Спасибо за советы! |
No description provided.