БЛОГ НА HUSL


Соответствие кода Laravel принципам SOLID

Автор вопроса: test

Соответствует ли код принципам SOLID?

Я создал следующие интефейсы:

  1. app/Interfaces/CarCreateInterface.php
<?php

namespace App\Interfaces;

use App\Models\Car;

interface CarCreateInterface
{
    /**
     * @param string $title
     * @return Car
     */
    public function create(string $title): Car;
}
  1. app/Interfaces/CarExistenceCheckerInterface.php
<?php

namespace App\Interfaces;

interface CarExistenceCheckerInterface
{
    /**
     * @param string $title
     * @return bool
     */
    public function exists(string $title): bool;
}
  1. app/Interfaces/CarManagementInterface.php
<?php

namespace App\Interfaces;

use App\Models\Car;

interface CarManagementInterface
{
    /**
     * @param string $title
     * @return Car
     */
    public function createIfNotExists(string $title): Car;
}

Затем я создал следующие сервисы:

  1. app/Services/CarCreateService.php
<?php

namespace App\Services;

use App\Interfaces\CarCreateInterface;
use App\Models\Car;

class CarCreateService implements CarCreateInterface
{
    /**
     * @param string $title
     * @return Car
     */
    public function create(string $title): Car
    {
        $car= new Car();
        $car->title = $title;
        $car->save();

        return $car;
    }
}
  1. app/Services/CarExistenceCheckerService.php
<?php

namespace App\Services;

use App\Interfaces\CarExistenceCheckerInterface;
use App\Models\Car;

class CarExistenceCheckerService implements CarExistenceCheckerInterface
{
    /**
     * @param string $title
     * @return bool
     */
    public function exists(string $title): bool
    {
        return Car::where('title', $title)
            ->exists();
    }
}
  1. app/Services/CarManagementService.php
<?php

namespace App\Services;

use App\Interfaces\CarCreateInterface;
use App\Interfaces\CarExistenceCheckerInterface;
use App\Interfaces\CarManagementInterface;
use App\Models\Car;
use Exception;

class CarManagementService implements CarManagementInterface
{
    protected CarExistenceCheckerInterface $car_existence_checker_service;
    protected CarCreateInterface $car_create_service;

    public function __construct(
        CarExistenceCheckerInterface $car_existence_checker_service,
        CarCreateInterface           $car_create_service,
    )
    {
        $this->car_existence_checker_service = $car_existence_checker_service;
        $this->car_create_service = $car_create_service;
    }

    /**
     * @param string $title
     * @return Car
     * @throws Exception
     */
    public function createIfNotExists(string $title): Car
    {
        if ($this->car_existence_checker_service->exists($title)) {
            throw new Exception("Машина уже добавлена");
        }

        return $this->car_create_service->create($title);
    }
}

Меня смущает что функция в сервисе CarManagementService объединяет в себе два функционала, проверка на существование и добавление, не нарушает ли мой код этим принципы SOLID?


Источник

Ответы (1 шт):

Автор решения: Total Pusher

Меня смущает что функция в сервисе CarManagementService объединяет в себе два функционала, проверка на существование и добавление, не нарушает ли мой код этим принципы SOLID?

Давайте пройдемся по всем буквам.

SRP

Каждый класс должен решать только одну задачу, быть "ответственным" за что-то одно - наверное как раз вас это и смутило.

Основная ошибка — считать, что "единственная ответственность" относится только к функциональности класса, то есть класс должен выполнять лишь одно действие. Однако это упрощение не совсем верно.

Что говорит Роберт Мартин (создатель SOLID)?

SRP означает, что у класса должна быть только одна причина для изменения. Это связано не столько с функциональностью, сколько с контекстом ответственности. Разные аспекты кода могут меняться по разным причинам, и задача SRP — изолировать эти изменения (чтобы не было, что небольшое изменение требований порождало массу изменений в куче классов, а наоборот было локализовано).

Давайте подумаем - что может измениться в системе, что потребует изменение реализации CarManagementService? Очень мало кода, поэтому придумываю.

  1. Добавить логирование действия по добавлению машины и уведомление об этом. Эти вещи прямо относятся к действию "добавить машину", поэтому могут быть встроены непосредственно в этот класс, и не нарушают SRP (даже подтверждают).
  2. При добавлении прицепа увеличивать метрику "количество прицепов в системе" - это изменение НЕ потребует изменения CarManagementService - как раз SRP в действии. А вот если потребует - значит SRP нарушен.

Open/Closed Principle

Классы должны быть открыты для расширения, но закрыты для изменения.

Другими словами, добавление нового функционала не должно требовать изменения уже существующего кода.

Никаких нарушений я не вижу. Вот что чекаю:

  1. private / final методы - их нет, можно расширять наследованием (но это не значит что если они есть - значит всё, нарушение, просто плюс к открытости)
  2. Dependency Injection (DI) - зависимости внедрены (также относится к другому принципу - Dependency Inversion Principle), а также можно использовать полиморфизм. На деле это значит, что можно через зависимость подставить другой полиморфный класс, в котором, например, при "create" слать уведомление (на самом деле - лучше в этом классе, либо через Event/Listener, но не буду углубляться)

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

Например, добавить создание события (патерн Event/Listener) на действие создания. Если добавить - будет плюс к открытости. Но также минус в качестве оверинжиниринга. Тут важно найти некоторый баланс между добавлением всяких Event/хуков/колбеков и оверинжирингом - это приходит с опытом. Вот мои рецепты:

  • Если делаете composer-пакет / библиотеку, то добавляйте - люди не смогут поменять что лежит в vendor, но через события смогут добавить свой функционал
  • Если это код, который завтра может быть снесен - не добавляйте
  • Если думаете, что код будет переиспользован, то добавляйте

Также если в базовом классе используется self::someMethod(), замените на static::someMethod() - Late Static Bindings позволит переопределить реализацию в потомке.

Liskov Substitution Principle

Не вижу наследования, поэтому ничего сказать нельзя.

Interface Segregation Principle

Вы нарезали функционал на тонкие интерфейсы, что согласуется.

Однако интерфейсы слишком тонкие - при таком подходе их будет очень много, избыточно много. Это одна крайность. Другая крайность - когда на большой сервис делается 1 интерфейс (и ISP соотвественно нарушается). Тут важен баланс, а также глубокое проникновение в предметную область.

Тут подходит паттерн repository.

Посмотрите vendor/laravel/framework/src/Illuminate/Contracts/Auth - там есть куча интерфейсов, которые относятся к одному домену/аспекту "авторизация", но нарезаны по функциональности:

  • Authenticatable - непосредственно авторизация
  • CanResetPassword - возможность сброса пароля (не во всех системах можно сбрасывать пароль - и это вынесено в отдельный интерфейс - тут как раз важно вникать в предметную область)
  • Guard - "защитник" - действия вокруг авторизованного/неавторизованного пользователя
  • MustVerifyEmail - необходимость подтверждать Email (опять же не всегда требуется, поэтому - отдельный интерфейс)
  • SupportsBasicAuth - поддержка basic-авторизации - по аналогии с MustVerifyEmail

DIP

Он у вас соблюден:

  1. Есть зависимость от абстракций (конкретно - интерфейсы) в более высоком CarManagementService
  2. Все сервисы зависят от абстракций, а не конкретных классов

Остальное (не критичное)

Замечание по DI

У вас зависимости в конструкторе, а значит что они создаются сервис-контейнером при создании класса. Однако $this->car_create_service->create($title) может никогда не вызываться. Поэтому лучше использовать app(CarCreateInterface::class)->....

Конкретно в данном случае это экономия на спичках (из соображения, что 99% будет вызываться), но в других случаях - лучшее решение. Также есть DI через методы-сеттеры - советую ознакомиться со всем списком вариантов.

Привязка к РСУБД

Есть жесткая сцепка с РСУБД из-за использования App\Models\Car в интерфейсе.

Если говорить не про SOLID, а про чистую архитектуру, то это нарушение - нужно абстрагироваться от фреймворка. Тут подходит шаблон repository + интерфейс "Машина".

Организация namespace

namespace App\Interfaces;
namespace App\Services;

Если ваша система вырастет до сотен тысяч строк кода, такая организация станет не удобной. В этом случае подходит доменная организация:

  • App\Domain\Car - интерфейсы домена "Машина"
  • App\Domain\Car\Models - модели
  • App\Domain\Car\Events - события
  • App\Domain\Car\Controllers - контроллеры
  • App\Domain\Car\FooBarSystem - интеграция с системой "FooBarSystem"

Рекомендую изучить Laravel Beyound CRUD (en) - очень хороший материал, использую практически все подходы на продакшн-системе.

Если сервис маленький и не будет расти - можете использовать "Ларовскую" схему организации классов.

Комментарии

На этом моменте я понял, что уже мой ответ походит на код-ревью :)

/**
 * @param string $title
 * @return Car
 */

Выкиньте эти комменты. Современные IDE и так все понимают, а вот вам придется поддерживать эти комменты.

Но есть редкие исключения - по этому моменту лучше прочитать любую книжку по чистому коду, глава "комментарии".

→ Ссылка

licensed under cc by-sa 3.0 with attribution.