From 5b4e71a0d06f858c6f4977af0b7580e140e6bac7 Mon Sep 17 00:00:00 2001 From: Mark Abramenko Date: Mon, 25 Jul 2022 13:54:25 +0300 Subject: [PATCH] CODE REVIEW --- lib/features/app/di/app_scope.dart | 8 + .../app/screens/main/main_screen_model.dart | 9 + lib/features/pizza/domain/entity/builder.dart | 161 ++++++++++++++++++ .../pizza/domain/entity/custom_pizza.dart | 34 +++- lib/features/platform/comment.md | 1 + .../test_start/test_start_screen.dart | 4 + 6 files changed, 216 insertions(+), 1 deletion(-) create mode 100644 lib/features/pizza/domain/entity/builder.dart create mode 100644 lib/features/platform/comment.md diff --git a/lib/features/app/di/app_scope.dart b/lib/features/app/di/app_scope.dart index 3e240ba..dc55e5e 100644 --- a/lib/features/app/di/app_scope.dart +++ b/lib/features/app/di/app_scope.dart @@ -35,6 +35,14 @@ class AppScope implements IAppScope { /// Create an instance [AppScope]. AppScope() { + /// CODE REVIEW + /// + /// Первое. Лучше вынести метод на уровень класса AppScope, чтобы не раздувать + /// код в конструкторе. + /// + /// Второе. Это хорошая возможность рассказать про смежный паттерн - + /// Фабричный метод. Можно посвятит 1-2 абзаца в статье различиям + /// Абстрактной фабрики и Фабричного метода, это будет очень кстати. PlatformWidgetsFactory _createPlatformWidgetsFactory() { switch (defaultTargetPlatform) { case TargetPlatform.android: diff --git a/lib/features/app/screens/main/main_screen_model.dart b/lib/features/app/screens/main/main_screen_model.dart index 958edd6..590736f 100644 --- a/lib/features/app/screens/main/main_screen_model.dart +++ b/lib/features/app/screens/main/main_screen_model.dart @@ -5,6 +5,15 @@ import 'package:pizza_elementary/features/platform/factory/platform_widgets_fact class MainScreenModel extends ElementaryModel { final PlatformWidgetsFactory _widgetsFactory; + /// CODE REVIEW + /// + /// Мне кажется, что инжектить фабрику для производства UI-компонентов через + /// Model - не очень хорошая идея. + /// + /// Model старается максимально абстрагироваться от того, что происходит + /// на UI. Даже если ты почитаешь документацию к классу [ErrorHandler], + /// который необходим для Модели, то увидишь, что там стоит восклицательный + /// знак на том, что его нельзя использовать для UI. PlatformWidgetsFactory get widgetsFactory => _widgetsFactory; MainScreenModel( diff --git a/lib/features/pizza/domain/entity/builder.dart b/lib/features/pizza/domain/entity/builder.dart new file mode 100644 index 0000000..4e5d453 --- /dev/null +++ b/lib/features/pizza/domain/entity/builder.dart @@ -0,0 +1,161 @@ +import 'package:pizza_elementary/features/pizza/domain/entity/ingredients_type.dart'; +import 'package:pizza_elementary/features/pizza/domain/entity/pizza_base.dart'; +import 'package:pizza_elementary/features/pizza/domain/entity/pizza_size.dart'; +import 'package:pizza_elementary/features/pizza/domain/entity/sauce_type.dart'; + +/// Первое. +/// +/// Конкретная модель должна быть абсолютно независима от билдера. +/// Проектировать её нужно так, будто конкретного билдера вовсе не существует. +/// +/// Билдер - всего-лишь инструмент. Конечная цель - получить из билдера +/// конкретную модель. +class PizzaProduct { + final PizzaBase base; + final SauceType sauce; + final List ingredients; + + PizzaProduct(this.base, this.sauce, this.ingredients); +} + +/// У Билдера есть 2 основных назначения: +/// +/// 1. Отделить мутабельную часть модели от иммутабельной. +/// 2. Позволить изменять при этом составные части объекта в разных местах +/// в разный промежуток времени. +/// 3. Иметь возможность контроллировать процесс сборки составных частей +/// базового объекта. +/// +/// Если пункт 1 можно рассматривать как сугубо утилитарный (в целом, для +/// его реализации можно использовать какой-нибудь переиспользуемный билдер), +/// то пункты 2 и 3 уже требуют тщательного подхода к реализации. +abstract class PizzaProductBuilder { + PizzaBase? base; + + SauceType? sauce; + + bool _isBuilt = false; + + List get _ingredients; + + void addBacon(); + + void addPepperoni(); + + void addHam(); + + PizzaProduct build() { + assert(!_isBuilt, 'Пицца уже была приготовлена'); + + if (base != null || sauce != null) { + throw Exception('Пицца не может быть без базы и соуса'); + } + + _isBuilt = true; + + return PizzaProduct( + base!, + sauce!, + _ingredients, + ); + } +} + +/// Билдер для данного случая у нас может ещё и выступать контроллером +/// ингредиентов, которые мы к нему добавляем. +class CustomPizzaProductBuilder extends PizzaProductBuilder { + @override + final List _ingredients = []; + + /// Предположим, что у нас здесь есть некие требования по количеству ингредиентов. + /// + /// Эти изменения в билдер можно даже добавлять извне. + @override + void addBacon() { + final baconCount = + _ingredients.where((i) => i == IngredientsType.bacon).length; + + if (baconCount > 3) { + throw Exception('Бекона не может быть более 3 штук'); + } + + _ingredients.add(IngredientsType.bacon); + } + + @override + void addHam() { + throw Exception('Нельзя добавлять говядину в пользовательскую пиццу'); + } + + @override + void addPepperoni() { + final pepperoniCount = + _ingredients.where((i) => i == IngredientsType.pepperoni).length; + + if (pepperoniCount > 3) { + throw Exception('Пепперони не может быть более 3 штук'); + } + + _ingredients.add(IngredientsType.pepperoni); + } +} + +class PepperoniPizzaProductBuilder extends PizzaProductBuilder { + /// Можем сразу добавить дефолтный ингредиент. + @override + final List _ingredients = [IngredientsType.pepperoni]; + + @override + void addBacon() { + throw Exception('Нельзя добавлять бекон в пепперони'); + } + + @override + void addHam() { + throw Exception('Нельзя добавлять говядину в пепперони'); + } + + @override + void addPepperoni() { + final pepperoniCount = + _ingredients.where((i) => i == IngredientsType.pepperoni).length; + + if (pepperoniCount > 2) { + throw Exception('В пепперони можнно сделать только двойную порцию'); + } + + _ingredients.add(IngredientsType.pepperoni); + } +} + +void example(PizzaProductBuilder builder) { + /// Пример обработки действия пользователя, который поменял базу и соус. + /// Как видишь, логику можно переиспользовать для билдера любой пиццы. + void onUserChangeBaseAndSauce(PizzaBase base, SauceType sauce) { + builder + ..base = base + ..sauce = sauce; + } + + /// Тут мы тоже можем переиспользовать логику добавления бекона, не обращая + /// внимание на конкретную реализацию метода для каждой возможно логики + /// бекона. + void onUserWantsToAddThreeBacons() { + try { + builder + ..addBacon() + ..addBacon() + ..addBacon(); + } on Exception catch (e) {} + } + + /// Обычный пример билда кастомной пиццы + final pizzaProduct = (builder + ..base = const PizzaBase(size: PizzaSize.M) + ..sauce = SauceType.tomato + ..addBacon() + ..addPepperoni() + ..addBacon() + ..addHam()) + .build(); +} diff --git a/lib/features/pizza/domain/entity/custom_pizza.dart b/lib/features/pizza/domain/entity/custom_pizza.dart index dbd801f..2aa3aff 100644 --- a/lib/features/pizza/domain/entity/custom_pizza.dart +++ b/lib/features/pizza/domain/entity/custom_pizza.dart @@ -3,10 +3,31 @@ import 'package:pizza_elementary/features/pizza/domain/entity/pizza_base.dart'; import 'package:pizza_elementary/features/pizza/domain/entity/pizza_size.dart'; import 'package:pizza_elementary/features/pizza/domain/entity/sauce_type.dart'; +/// CODE REVIEW +/// +/// По моему мнению, это самая слабая часть статьи. Здесь билдер существует как +/// будто только для того, чтобы добавить его в статью. При этом абсолютно нет +/// примеров, чем этот паттерн может помочь в разработке. Свой пример реазиации +/// оставил в файле builder.dart + /// Пицца, которую собирает пользователь /// [title] - название базовой пиццы c бэка /// [ingredients] - дополнительные ингредиенты что пользователь добавляет к базовой пицце class CustomPizza { + /// CODE REVIEW + /// + /// Кажется, у тебя возникла проблема с использованием паттерна именно из-за + /// недостаточной декомпозиции. + /// + /// Такое ощущение, что все поля этой модели просто используются в разных + /// местах. + /// + /// Я предлагаю разделить это на 2 модели: PizzaData и PizzaProduct. + /// В PizzaData буду храниться данные о пицце как наборе данных: название, + /// изображение, цена. При этом составная часть пиццы должна быть отдельным + /// параметром в модели. В PizzaProduct буду храниться ингредиенты, размер, + /// соус и всё остальное. Такое ощущение, что именно для PizzaProduct нужен + /// билдер, а для всего остального - нет. late final String title; late final PizzaBase base; late final SauceType sauce; @@ -16,6 +37,9 @@ class CustomPizza { static PizzaBuilder get builder => PizzaBuilder(); + /// CODE REVIEW + /// + /// Модель не должна быть зависима от способа создания. CustomPizza(PizzaBuilder builder) { title = builder.title; base = builder.base; @@ -60,6 +84,13 @@ class PizzaBuilder { String get imageUrl => _imageUrl; + /// CODE REVIEW + /// + /// Очень не нравится подход с использованием дефолтных значений вместо null. + /// + /// Тут есть простая лакмусовая бумажка. Задай себе вопрос. Хочешь ли ты + /// увидеть дефолтные значения в приложении, которым ты пользуешься? И + /// почему ты можешь увидеть именно те значения, которые стоят в дефолтных? String _title = ''; PizzaBase _base = const PizzaBase(size: PizzaSize.M); SauceType _sauce = SauceType.tomato; @@ -77,7 +108,8 @@ class PizzaBuilder { void setSauce(SauceType sauce) => _sauce = sauce; // ignore: use_setters_to_change_properties - void setIngredients(Set ingredients) => _ingredients = ingredients; + void setIngredients(Set ingredients) => + _ingredients = ingredients; // ignore: use_setters_to_change_properties void setPrice(int price) => _price = price; diff --git a/lib/features/platform/comment.md b/lib/features/platform/comment.md new file mode 100644 index 0000000..f561301 --- /dev/null +++ b/lib/features/platform/comment.md @@ -0,0 +1 @@ +Неясно, почему директория "platform" это фича? Я бы вынес это вообще в отдельную директорию на уровне /lib и никак не связывал с фичами. \ No newline at end of file diff --git a/lib/features/test_start/test_start_screen.dart b/lib/features/test_start/test_start_screen.dart index 3f0f126..c69c649 100644 --- a/lib/features/test_start/test_start_screen.dart +++ b/lib/features/test_start/test_start_screen.dart @@ -1,5 +1,9 @@ import 'package:flutter/material.dart'; +/// CODE REVIEW +/// +/// Кажется, этот экран нужно убрать отсюда + /// Стартовый экран для теста запуска приложения class TestStartScreen extends StatelessWidget { const TestStartScreen({Key? key}) : super(key: key);