Vladislav Pirtan #3
Conversation
PythonAndGoCommunity
force-pushed
the
master
branch
from
b5a4c19 to
17385ed
Compare
| # global variable dict, key: str module name, value: list containing fuction names from [key] | ||
| module_func_dict = {} | ||
| # Operator stack | ||
| stack_opr = [] |
There was a problem hiding this comment.
Глобальные переменные стоит использовать с большой осторожностью. В данном случае лучше было бы создать класс, сделать данную переменную атрибутом объекта и ограничить область использования этого атрибута методами класса.
There was a problem hiding this comment.
Насколько можно судить, эти переменные используются для того, чтобы "эмулировать" поведение класса. Так почему бы просто не использовать класс в этом случае? :)
| def set_user_mod(modules=None): | ||
| """set_user_mod(modules) | ||
|
|
||
| Initialize global variable module_func_dict |
There was a problem hiding this comment.
По всему коду по какой-то причине "съехала" табуляция на документации к функции.
Также документация повторяет название функции, что бессмысленно. Он может нести больше полезной информации, либо докстринг можно опустить, чтобы не захламлять код.
| buf = list() | ||
| # right-associative operator | ||
| if opr == '^' or opr == '+-': | ||
| for opr2 in stack_opr[::-1]: |
There was a problem hiding this comment.
opr2 -- плохое название для переменной. Пока программист не прочитает весь код функции и не поймет как она работает, он не поймет, что значит оператор 2, хотя наззвание переменных как раз и должны помогать понимать код.
| if type(result) is complex: | ||
| return 'ERROR: negative number cannot be raised to a fractional power' | ||
| stack.append(result) | ||
| if token == '+-': |
There was a problem hiding this comment.
Очень большое дублирование кода. Для таких случаев лучше иметь словарь, ключами которого будут являться токены, а значением нечто, что может выполнить желаемое действие, например функции из модуля operator, лямбда выражения или вообще какие-либо объекты.
| i = 0 | ||
| l_const = '' | ||
| while i < len(const): | ||
| l_const += const[i] |
There was a problem hiding this comment.
Если переменная называется const, программист как минимум будет ожидать, что это константа, но никак не список.
| raise Exception('empty input') | ||
|
|
||
| output_list = list() | ||
| func_buf = list() # argument counter in function |
There was a problem hiding this comment.
Так а что является счетчиком аргументов: func_buf или count_args?
| func_buf = list() # argument counter in function | ||
| last_token = '' | ||
| count_args = list() # argument counter in function | ||
| for i, token in enumerate(input_str): |
There was a problem hiding this comment.
Лучше избегать названий переменных из одного символа. Даже если эта переменная обозначает индекс, лучше написать index вместо i
| output_list.extend(_const_separator(''.join(func_buf))) | ||
| func_buf = [] | ||
| # unary operator | ||
| if (token == '-') & (last_token in LIST_OPERATOR or last_token in ['', '(']): |
There was a problem hiding this comment.
and и & -- это разные вещи. Первый оператор делает логическое "и", второй побитовое. В данном случае это работает, однако тут надо использовать and.
PythonAndGoCommunity
left a comment
PythonAndGoCommunity
left a comment
There was a problem hiding this comment.
Довольно сложно выделить какие-то отдельные проблемные моменты. Решение нуждается в декомпозиции. Разбор математического выражения и его вычисление - довольно сложная, комплексная задача. Несмотря на это она решена при помощи всего 6 функций. Ожидаемо, логика функций и логика проекта в целом очень сложна для понимания. Осложняют все следующие моменты:
- Имена переменных редко помогают понять их назначение
- Активно используются глобальные переменные
- Высокая степень дублирования кода
- Крайне высокая степень связности кода
Что можно было бы предпринять:
- Грамотно разделить всю логику приложения на отдельные, маленькие, независимые, по возможности чистые функции с четкой и простой задачей. Keep it simple, stupid!
- проанализировать полученные функции и по возможности привести более частные случаи к более общим, что помогло бы сократить дублирование.
2 participants
No description provided.