Andrey Mirugin #19
Andrey Mirugin #19
Conversation
| is_unar = False | ||
| end = -1 | ||
| result_expression = expression | ||
| for i in range(len(result_expression)): |
There was a problem hiding this comment.
В данном случае лучше использовать цикл в таком виде:
for index, value in enumerate(result_expression):
...Читается лучше и нет необходимости обращаться к элементам коллекции по индексам.
| result_expression = expression | ||
| for i in range(len(result_expression)): | ||
| if result_expression[i] == "+" or result_expression[i] == "-": | ||
| if not is_unar: |
| return result_expression | ||
|
|
||
|
|
||
| def checking_and_solving_comparison(expression): |
There was a problem hiding this comment.
- в данном случае функцию лучше назвать
check_and_solve_comparison - Если названии функции отмечено два действия -- это уже звоночек, что функцию необходимо разбить на несколько маленьких. Функция не должна делать несколько разных действий. В идеале она должна быть максимально простой и желательно чистой
| is_comparison = True | ||
| return [comparison[expression[i]+expression[i+1]](calc(expression[0:i]), | ||
| calc(expression[i+2:len(expression)])), is_comparison] | ||
| return [expression, is_comparison] |
There was a problem hiding this comment.
Еще один звоночек, что функция достаточно сложная, -- это возвращение нескольких значений.
В случае, если очень-очень необходимо вернуть из функции два значения, лучше использовать namedtuple или стандартный кортеж.
There was a problem hiding this comment.
Лучше строить логику функции так, чтобы в ней было наименьшее количество точек выхода из функции. В идеале только один return в конце функции.
| first_element = "" | ||
| start = 0 | ||
| for i in range(1, pointer+1): | ||
| prev = first_element |
There was a problem hiding this comment.
previous
неколько симоволов не сделают погоды :)
сокращение не имеет смысла в этом случае
| End position of element | ||
| """ | ||
| end = 0 | ||
| flag = False |
There was a problem hiding this comment.
Если необходимо в коде функции использовать флаг, то нужно дать этому флагу осмысленное имя. Человек, который будет читать этот код, не поймет, что значит этот флаг, пока не поймет всю функцию.
| expression = string.EXPRESSION | ||
| expression = replace_spaces(expression) | ||
| expression = add_implicit_mult(expression) | ||
| [expression, is_comparison] = checking_and_solving_comparison( |
There was a problem hiding this comment.
Так делать не принято. Распоковка не требует скобочек в выражении с левой стороны, либо, если очень хочется, можно поставить круглые скобки:
(expression, is_comparison) = checking_and_solving_comparison(...)| res_exp = expression | ||
| space_pos = res_exp.find(" ") | ||
| while space_pos != -1: | ||
| if space_pos-1 >= 0 and res_exp[space_pos-1] in ("+", "-", "*", "^", "%", "=", ">", "<", "!", "/", ","): |
There was a problem hiding this comment.
В этой функции есть много перечислений операторов, которые имеют какой-то сокральный смысл, недоступный непосвященному :)
Как минимум есть смысл дать всем этим перечислениям осмысленное название.
| def calc(expression): | ||
| """Calculate expression with no spaces""" | ||
| result_expression = expression | ||
| result_expression = replace_long_unaries(result_expression) |
There was a problem hiding this comment.
Что ж такое произошло, что функцию replace_long_unaries нужно вызывать 4 раза?
There was a problem hiding this comment.
Каждая замена, скобок на их значение и подобные, могла породить унарные знаки. replace_long_unaries убирает их. Поэтому после каждого действия я вызывал её
| elif is_float(expr_right): | ||
| was_float = True | ||
| elif not is_float(expr_right) and was_float: | ||
| result_expression = result_expression[0:i] + \ |
There was a problem hiding this comment.
- Модификация коллекции во время итерирования по этой коллекции с помощью цикла
for-- крайне плохая идея. - Спагетти-код, который крайне сложно понять
| maxprior = priority[expression[i]] | ||
| result = calc_by_position_of_sign(position, expression) | ||
| new_string = expression.replace( | ||
| expression[result[1]:result[2]+1], str("{:.16f}".format(result[0]))) |
There was a problem hiding this comment.
Без карандаша и листика невозможно представить в голове, что из этого получится.
PythonAndGoCommunity
left a comment
PythonAndGoCommunity
left a comment
There was a problem hiding this comment.
Некоторые реализованные функции выглядят громоздко и выполняют слишком много задач. Нужно стремиться к условной атомарности функции, т.е. чтобы функция выполняла одну задачу. В этом случае она будет более универсальна и её можно будет применить где-нибудь ещё.
Функции желательно проектировать так, чтобы они не возвращали разные типы данных (str или int, к примеру) в зависимости от результата их работы. Функция которая возвращает %data type%, None или рейзит ошибку - хороший пример.
2 participants
No description provided.