Aliaksei Karatynski by astronomer-alkor · Pull Request #8 · PythonAndGoCommunity/PythonHomework

astronomer-alkor · 2018-11-18T19:45:33Z

No description provided.

Signed-off-by: Aliaksei Buziuma <[email protected]>

.

PythonAndGoCommunity · 2019-01-30T16:08:48Z

final_task/pycalc/core/args.py

+import argparse
+
+
+def arg_parser():


Согласно негласным соглашениям хорошим тоном считается давать функциям названия, выражающие действие (глагол). Название parse_args подошло бы лучше.
Кроме того, объем и сложность логики данной функции очень малы для вынесения в отдельный модуль. Обычно такую функцию называют _parse_args и располагают в модуле, являющемся точкой входа приложения.

PythonAndGoCommunity · 2019-01-30T16:12:27Z

final_task/pycalc/core/calculator.py

+    """
+    Import modules
+    """
+    sys.path.insert(0, os.path.abspath(os.path.curdir))


Костыль! :)

PythonAndGoCommunity · 2019-01-30T16:16:37Z

final_task/pycalc/core/calculator.py

+
+def split_operands(merged_operand):
+    """
+    Split operands


Док стринг повторяет название функции, что бессмысленно. Он может нести больше полезной информации, либо докстринг можно опустить, чтобы не захламлять код.
если есть желание уточнить принимаемый/возвращаемый типы, можно использовать тайп хинты https://docs.python.org/3/library/typing.html

PythonAndGoCommunity · 2019-01-30T16:18:07Z

final_task/pycalc/core/calculator.py

+)
+
+PRIORITIES = {
+    1: ['<', '>', '<=', '>=', '==', '!='],


Вместо списка для констант лучше использовать кортежи. Таким образом мы подчеркиваем, что данное перечисление не планируется изменять и используем меньше памяти.

PythonAndGoCommunity · 2019-01-30T16:20:30Z

final_task/pycalc/core/calculator.py

+    token_end_position = -1
+    insert_positions = []
+    expression = expression.replace(' ', '')
+    expression = re.sub(r'\)\(', ')*(', expression)


Регулярные выражения лучше выносить в отдельные переменные, чтобы программист, который читает код, смог получить дополнительную информацию о происходящем из названия переменной. Чтение названия переменной занимает намного меньше времени, чем попытка осознать смысл регулярного выражения.

PythonAndGoCommunity · 2019-01-30T16:25:11Z

final_task/pycalc/core/calculator.py

+    expression = list(expression)
+    for index in insert_positions:
+        expression.insert(index, '*')
+    return ''.join(expression)


Судя по всему в этой функции выполняется неявная токенизация, подстановка знаков умножения, а затем обратный перевод в строку. Зачем делать одну и ту же работу два раза?

PythonAndGoCommunity · 2019-01-30T16:28:26Z

final_task/pycalc/core/calculator.py

+            if module == builtins and attribute not in BUILTINS_FUNCS:
+                continue
+            return module
+    return


Зачем?

PythonAndGoCommunity · 2019-01-30T16:36:53Z

final_task/pycalc/core/calculator.py

+    :param expression
+    :param token_end_position
+    :param module
+    :return: the constant or result of the function and


Функция должна обладать единым четким и понятным интерфейсом. Результат работы функции должен интерпретироваться единообразно.

PythonAndGoCommunity · 2019-01-30T16:44:11Z

final_task/pycalc/core/calculator.py

@@ -0,0 +1,487 @@
+"""


Код нуждается в декомпозиции. Например разделить модуль calculate на несколько маленьких модулей или инкапсулировать функциональность в несколько разных классах, некоторые большие функции разделить на несколько маленьких, где это возможно

PythonAndGoCommunity · 2019-01-30T16:48:01Z

final_task/pycalc/main.py

+    answer = do_calculation(arguments.EXPRESSION, arguments.MODULE)
+    print(answer)
+    if str(answer).startswith('ERROR'):
+        sys.exit(-1)


Почему -1? Не соответствует конвенции UNIX описывающей возвращаемые значения программы (от 0 до 255)

PythonAndGoCommunity · 2019-01-30T16:53:34Z

final_task/pycalc/core/calculator_helper.py

+    return operator == '^'
+
+
+def check_is_callable(attribute, module):


В чем смысл этой функции? Она используется в одном месте в бизнес логике приложения (исключая юнит тесты) и не содержит какую-либо существенную логику.

PythonAndGoCommunity · 2019-01-30T16:58:55Z

final_task/setup.py

@@ -0,0 +1,14 @@
+"""
+This module allows to installs the application


Слишком очевидно чтобы быть тут.

PythonAndGoCommunity · 2019-01-31T15:54:24Z

final_task/pycalc/core/calculator.py

+import sys
+import operator as op
+from .calculator_helper import PycalcError
+from .calculator_helper import (


В случаях, когда импортируется множество сущностей из одного и того же модуля, принято распологать их по одному на строку, для повышения читаемости.

from .calculator_helper import ( check_is_number, check_may_unary_operator, ... check_brackets_balance, )

PythonAndGoCommunity · 2019-01-31T15:55:20Z

final_task/pycalc/core/calculator.py

+import os
+import re
+import sys
+import operator as op


Сомнительная выгода с точки зрения объема кода, но понижение читаемости существенное.
Кроме того в твоем коде активно используется имя operator, что может запутать программиста, который может ожидать от этого имени использование стандартной библиотеки

PythonAndGoCommunity · 2019-01-31T16:02:46Z

final_task/pycalc/main.py

+    """
+    arguments = args.arg_parser()
+
+    answer = do_calculation(arguments.EXPRESSION, arguments.MODULE)


Вместо возвращения объекта ошибки из функции do_calculation логичнее отлавливать исключения на самом высоком уровне:

try: print(do_calculation(arguments.EXPRESSION, arguments.MODULE)) except PycalcError as e: print(e) except Exception as e: print("ERROR: {}".format(e))

PythonAndGoCommunity · 2019-01-31T16:04:34Z

final_task/pycalc/core/calculator.py

+    return True
+
+
+def check_valid_spaces(expression):


Название и описание функции не соответствует ее назначению. Функция явно в целом проверяет валидность выражения, а не только пробелы.

PythonAndGoCommunity · 2019-01-31T16:06:40Z

final_task/pycalc/core/calculator_helper.py

+        float(string)
+        return True
+    except ValueError:
+        return


Лучше в данном случае вернуть False

PythonAndGoCommunity · 2019-01-31T16:13:19Z

final_task/pycalc/core/calculator.py

+    check_valid_spaces(expression)
+
+
+def update_operands(expression, operands, index):


Ни из названия функции, ни из ее описания не ясно что она делает. Кроме обновления операндов она считает некий token_end_position. Это явно указывет на ошибки, совершенные при декомпозиции (разбиении программы на отдельные независимые сущности). То же самое для функции update_operators (строка 401).

PythonAndGoCommunity · 2019-01-31T16:19:00Z

final_task/pycalc/core/calculator.py

+    """
+    for operator in operators:
+        if get_priority(operator) > 1:
+            return


Несоответствие с докстрингом, в данном случае функция возвращает None

PythonAndGoCommunity · 2019-01-31T16:27:07Z

final_task/pycalc/core/calculator.py

+            condition_b = get_priority(operators[-1]) >= get_priority(operator)
+            condition_c = check_right_associativity(operator)
+            condition_d = get_priority(operators[-1]) > get_priority(operator)
+            condition = condition_a and condition_b or condition_c and condition_d


Плохо. Такие блоки кода очень сложно понять человеку, не знакомому с проектом. Если бы декомпозиция была проведена грамотно, в функции не было бы такого количества условий.

PythonAndGoCommunity · 2019-01-31T16:33:09Z

В целом в проекте прослеживается одна и та же проблема: декомпозиция (разбиение логики проекта на отдельные независимые сущности) проведена не лучшим способом. Из-за этого появились функции, выполняющие сразу несколько обязанностей. Из-за этого становится сложно подобрать функции верное и понятное название, сложно выбрать простой и четкий интерфейс. Код становится запутанным.

AliakseiBuziuma and others added 8 commits November 10, 2018 18:14

update CI script and hometask

3e2ed9b

Signed-off-by: Aliaksei Buziuma <[email protected]>

Merge remote-tracking branch 'upstream/master'

7fffab0

.

add pycalc

a53cf47

remove tests

6ea7324

fix setup

c9ae1f8

remove setup

5c49201

change version

0ca1dd4

fix imports

03090b6

astronomer-alkor changed the title ~~[WIP] Karatynski Aliaksei~~ [WIP] Aliaksei Karatynski Nov 19, 2018

astronomer-alkor added 7 commits November 24, 2018 03:44

add tests & code refactoring

3f41c81

fix bug

91fd339

add tests

f504e6d

fix bug

9f96f03

code refactor

19ef8e3

code refactor

1794931

code refactor

0ccfeba

astronomer-alkor changed the title ~~[WIP] Aliaksei Karatynski~~ Aliaksei Karatynski Dec 16, 2018

astronomer-alkor added 3 commits December 20, 2018 04:13

code refactor

8dbbade

fix bug

ab336d8

fix bug

bf0a645

PythonAndGoCommunity added the Code freeze label Dec 20, 2018

PythonAndGoCommunity reviewed Jan 30, 2019

View reviewed changes

PythonAndGoCommunity approved these changes Jan 30, 2019

View reviewed changes

PythonAndGoCommunity reviewed Jan 31, 2019

View reviewed changes

PythonAndGoCommunity added the Reviewed label Jan 31, 2019

		return operator == '^'


		def check_is_callable(attribute, module):

		@@ -0,0 +1,14 @@
		"""
		This module allows to installs the application

		check_valid_spaces(expression)


		def update_operands(expression, operands, index):

Conversation

astronomer-alkor commented Nov 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PythonAndGoCommunity Jan 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PythonAndGoCommunity Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PythonAndGoCommunity Jan 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PythonAndGoCommunity commented Jan 31, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

PythonAndGoCommunity Jan 30, 2019 •

edited

Loading

PythonAndGoCommunity Jan 31, 2019 •

edited

Loading

PythonAndGoCommunity Jan 31, 2019 •

edited

Loading