Skip to content

K kamischin/hw13#234

Open
inc-lab wants to merge 5 commits into
KKamischin/hw12from
KKamischin/hw13
Open

K kamischin/hw13#234
inc-lab wants to merge 5 commits into
KKamischin/hw12from
KKamischin/hw13

Conversation

@inc-lab

@inc-lab inc-lab commented Aug 28, 2025

Copy link
Copy Markdown

No description provided.


namespace App\Domain\News\Repositories;

use App\Domain\News\Entities\News as DomainNews;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Тут вроде других News нет.. можно было и не делать alias

* @param int $limit
* @param int $offset
*
* @return array

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

array<array-key,News> или News[]

public function fetchAll(): array;

/**
* @param int $limit

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Смысл дублировать - есть нативные типы


namespace App\Domain\News\Services;

interface CategorySlugGeneratorInterface

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


use Exception;

final class UserNotFoundException extends Exception

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Для доменных исключений есть базовый класс DomainException, лучше от него наследоваться


namespace App\Domain\User\ValueObjects;

final class Permissions

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

не очень удачный кандидат на ValueObject, обычно VO не бывают коллекциями..


final class Permissions
{
public function __construct(public array $permissions) {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

не указан тип значений массива

* Create a new event instance.
*/
public function __construct(News $news)
public function __construct(readonly public int $id, readonly public string $title, readonly public string $content)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

нечитабельно. Поставьте себе пакет php-cs-fixer, он вам сам будет стиль править по psr

->limit($limit)
->get();

return array_map([NewsMapper::class, 'toEntity'], $models->all());

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

методы строковыми литералами вам рефакторинг превратят в ад, уж лучше коллбек функцию передать и там явно дергать Mapper::toEntity()

$model->{$model->getColumnName('id')} = $news->getId();
}

$model->{$model->getColumnName('title')} = $news->getTitle();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Вот тут не понимаю, зачем вы эту конструкцию используете?
$model->{$model->getColumnName('title')} это ж адок в плане читабельности...
В eloquent к свойствам обращаются так: $model->title.
Если бы вы трижды не проигнорировали мой комментарий в предыдущей домашке по разметке сущности доклблоками @property string $title вам было бы тут гораздо проще.

use App\Models\Category as EloquentCategory;
use App\Models\User as EloquentUser;

class NewsMapper

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

мэппер - отлично

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants