Create Pizzeria_oop by Seniacat · Pull Request #3 · smartiqaorg/python_code_reviewer

Seniacat · 2021-04-20T19:34:37Z

No description provided.

t-kutenkova · 2021-04-21T11:47:27Z

Pizzeria_oop

@@ -0,0 +1,63 @@
+class Pizza:


В целом по программе: необходимо разобраться в пустыми строками. Как правильно, Style Guide для Python находится в официальном документе PEP8 - в нем подробно описано, как нужно писать читабельный код: https://www.python.org/dev/peps/pep-0008

Так вот касательно пустых строк в нем написано так:

Surround top-level function and class definitions with two blank lines.
Method definitions inside a class are surrounded by a single blank line.
Extra blank lines may be used (sparingly) to separate groups of related functions.

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

Руководствуясь этими принципами, поправьте пустые строки в вашей программе.

Fixed. Честно говоря,стараюсь не писать вот так в одну строку функции. Но тут показалось,что вышла портянка,хотела уменьшить визуально ее,но получилось только хуже. Исправляюсь)

t-kutenkova · 2021-04-21T11:59:30Z

Pizzeria_oop

@@ -0,0 +1,63 @@
+class Pizza:
+    pizza_size = {0: "SMALL", 1: "MEDIUM", 2: "LARGE"}


Смысл констант для размеров пиццы был в том, чтобы хранить в них конкретные величины размеров. Как я уже объясняла это для видов пиццы, тут такой же принцип: мы имеем три константы для размеров (25см 30см и 35см). И если вдруг наша большая пицца станет не 35, а 36см, то нам нужно будет этот размер изменить всего в одном месте.

У вас для размеров словарь(и самих размеров нет). И получается вот что:
Ваш словарь: pizza_size = {0: "SMALL", 1: "MEDIUM", 2: "LARGE"}
Использование словаря: Pizza.pizza_size[0]

А вот через использование констант:
Константы:

SMALL = 25 MEDIUM = 30 LARGE = 35

Их использование:
Pizza.SMALL

Видите в чем плюс использования констант? Такая запись Pizza.pizza_size[0] не дает вам представления, про какой размер пиццы мы говорим - здесь просто [0]. То есть чтобы другому программисту понять, что это за ноль такой, ему придется смотреть содержимое словаря. А вот если мы используем константы, то SMALL уже сразу говорит, что мы работаем с пиццей маленького диаметра.

Теперь поняла. До этого смысл этих статических полей ускользал от меня.

t-kutenkova · 2021-04-21T12:10:52Z

Pizzeria_oop

+class Pizza:
+    pizza_size = {0: "SMALL", 1: "MEDIUM", 2: "LARGE"}
+    pizza_type = {"PEPPERONI": "Pepperoni", "MARGARITA": "Margarita"}
+    ready = {0: "COOKING", 1: "READY"}


Такая же история, как и с другими словарями. Лучше сделать так:

COOKING = 0 READY = 1

t-kutenkova · 2021-04-21T12:22:37Z

Pizzeria_oop

+        self.size = size
+        self.ready = Pizza.ready[0]
+
+    def cook(self): setattr(self, 'ready', Pizza.ready[1])


[ Касается всей программы] Содержимое функции переносим на следующую строку, даже если это содержимое длиной в 1 строчку. Почему? Так читабельнее.

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

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

Это особенно важно, потому что у нас питон, а в нем даже скобки не предусмотрены. То есть например в js можно по разному с кодом функции извращаться за счет скобок(те же самые стрелочные функции), но в питоне так делать не надо.

В чем смысл использовать функцию setattr()? Разве self.ready = Pizza.ready[1] не компактнее и привычней для понимания? Плюс если поправить еще константы, то это вообще превратится в такую строку: self.state = READY
А сама функция может выглядеть так:

def cook(self): self.state = COOKING print('Pizza is being cooked...') self.state = READY print('Pizza is ready!')

Fixed. Правда,метод cook() поняла по-другому. Т.е. для меня экземпляр пиццы создается уже сразу готовящимся,а метод cook() просто переводит пиццу из состояния "в печи" в состояние "готово".Наверно,это не кричтично, оставила свой вариант пока

t-kutenkova · 2021-04-21T12:31:54Z

Pizzeria_oop

+class Worker:
+    def __init__(self):
+        self.order = None
+        self._cash = 0


По заданию данное поле должно быть приватным

исправила

t-kutenkova · 2021-04-21T12:35:11Z

Pizzeria_oop

+    def greet_client(name): print(f"Welcome, {name}!")
+
+    def get_order(self, pizza_type, pizza_size):
+        if pizza_type == 'Pepperoni':


Здесь мы возвращаемся к нашей истории с константами для названий пиццы. Использование произвольных строк вообще очень нежелательно в коде (это я про 'Pepperoni'), потому что 1) легко можно допустить опечатку и 2) если будут изменения, то придется править такие строки по всей программе.

Поэтому здесь было бы уместнее взять константу для нужного типа пиццы:
if pizza_type == Pizza.PEPPERONI:

t-kutenkova · 2021-04-21T12:36:44Z

Pizzeria_oop

+        return self.order
+
+class Client:
+    client_name = 'Alex'


в названии неплохо бы отразить, что это дефолтное имя клиента

t-kutenkova · 2021-04-21T12:37:21Z

Pizzeria_oop

+class Client:
+    client_name = 'Alex'
+
+    def __init__(self, money, name=client_name,):


Запятая

t-kutenkova · 2021-04-21T12:39:05Z

Pizzeria_oop

+    client_name = 'Alex'
+
+    def __init__(self, money, name=client_name,):
+        self._money = money


по заданию здесь должно быть приватное поле

t-kutenkova · 2021-04-21T12:46:14Z

Pizzeria_oop

+        super().__init__(Pepperoni.ings, size)
+
+class Margarita(Pizza):
+    ings = ['Tomato', 'Cheese']


Переменная ings по смыслу конфликтует в полем ingredients из родительского класса. Получается, что класс Pepperoni имеет свое поле ings плюс еще наследует из родительского класса поле ingredients. Названия практически идентичные. Тут можно либо

Переменную ings перенести внутрь инициализатора (метода init()), то есть просто сделать ее локальной

либо

В названии ings отразить, что это именно ингредиенты пиццы типа пепперони.

Create Pizzeria_oop

f18c947

t-kutenkova suggested changes Apr 21, 2021

View reviewed changes

Seniacat added 2 commits April 21, 2021 17:14

Update Pizzeria_oop

419689f

Update Pizzeria_oop

b114d2b

smartiqa approved these changes Apr 21, 2021

View reviewed changes

		@@ -0,0 +1,63 @@
		class Pizza:
		pizza_size = {0: "SMALL", 1: "MEDIUM", 2: "LARGE"}

Conversation

Seniacat commented Apr 20, 2021

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants