Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CODE REVIEW #10

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions lib/features/app/di/app_scope.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
9 changes: 9 additions & 0 deletions lib/features/app/screens/main/main_screen_model.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
161 changes: 161 additions & 0 deletions lib/features/pizza/domain/entity/builder.dart
Original file line number Diff line number Diff line change
@@ -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<IngredientsType> 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<IngredientsType> 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<IngredientsType> _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<IngredientsType> _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();
}
34 changes: 33 additions & 1 deletion lib/features/pizza/domain/entity/custom_pizza.dart
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -16,6 +37,9 @@ class CustomPizza {

static PizzaBuilder get builder => PizzaBuilder();

/// CODE REVIEW
///
/// Модель не должна быть зависима от способа создания.
CustomPizza(PizzaBuilder builder) {
title = builder.title;
base = builder.base;
Expand Down Expand Up @@ -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;
Expand All @@ -77,7 +108,8 @@ class PizzaBuilder {
void setSauce(SauceType sauce) => _sauce = sauce;

// ignore: use_setters_to_change_properties
void setIngredients(Set<IngredientsType> ingredients) => _ingredients = ingredients;
void setIngredients(Set<IngredientsType> ingredients) =>
_ingredients = ingredients;

// ignore: use_setters_to_change_properties
void setPrice(int price) => _price = price;
Expand Down
1 change: 1 addition & 0 deletions lib/features/platform/comment.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Неясно, почему директория "platform" это фича? Я бы вынес это вообще в отдельную директорию на уровне /lib и никак не связывал с фичами.
4 changes: 4 additions & 0 deletions lib/features/test_start/test_start_screen.dart
Original file line number Diff line number Diff line change
@@ -1,5 +1,9 @@
import 'package:flutter/material.dart';

/// CODE REVIEW
///
/// Кажется, этот экран нужно убрать отсюда

/// Стартовый экран для теста запуска приложения
class TestStartScreen extends StatelessWidget {
const TestStartScreen({Key? key}) : super(key: key);
Expand Down