odan / slim4-skeleton

A Slim 4 Skeleton
https://odan.github.io/slim4-skeleton/
MIT License
439 stars 80 forks source link

Way of communication #63

Closed DigiLive closed 3 years ago

DigiLive commented 3 years ago

Is there a way of communication to speak/ask question about this skeleton? I have some "generic" questions, but I don't want to abuse the issue system.

(Maybe by enabling the Discussions feature in this repos settings)

odan commented 3 years ago

No problem, you can create an issue with your generic question here.

DigiLive commented 3 years ago

Thank you very much :)

Building upon this skeleton I've noticed there's a lot of code duplication if you follow the ADR pattern of the user which is provided in the skeleton.

E.g. In addition of users, I have items (more will follow) which have pretty much the same actions, domain and responder. They mostly differ in properties/fields I read/write.

What's best practice for extending the API for other items?

Things that have crossed my mind...

  1. Should I make a copy from the user "ADR", alter the the variable names/types, etc. so it's suitable for item? This means a lot of code duplication which I don't like.

  2. Should I create (abstract) classes and extend them for user and item? This means coding becomes more cryptic , because users and items use different services, etc. return $this->transform($response, $this->finderService->find($request->getQueryParams())); finderService can be UserFinder or ItemFinder or anything else, only known at runtime. Nor me, nor the IDE can resolve the method onbeforehand.

  3. Should I create factories? Probable a factory needs a lot of code to determine what to deliver, depending on needing users, items. etc.

odan commented 3 years ago

I would like to answer your question in detail, but first I have to understand what do you mean with "redundant code". From a use case perspective there is no redundant code and from a source-code perspective there is also no redundant code (according to the CI code analyzer). So what is exactly redundant here? Can you please show me an example of redundant code?

DigiLive commented 3 years ago

I hope I can clarify...

The API handles users. If we want a list of users, we go trough the following method and files:

  1. $app->get('/users', UserFindAction::class); calls App\Action\User\UserFindAction
  2. App\Action\User\UserFindAction::__construct() get a UserFinder service.
  3. App\Action\User\UserFindAction::__invoke() calls App\Domain\User\Service\UserFinder::findUsers().
  4. App\Domain\User\Service\UserFinder::__construct() gets a App\Domain\User\Repository\UserFinderRepository repo.
  5. App\Domain\User\Service\UserFinder::__findUsers() calls App\Domain\User\Repository\UserFinderRepository::findUsers().
  6. App\Domain\User\Repository\UserFinderRepository::construct() gets App\Factory\QueryFactory.

Now I want to a getting a list of animals to the API. Essentially I think we can can apply the same as above. Copy the User files, rename User to Animal so file/directory names, namespaces, class/method names are correct and imply the handling of animals. Besides those differences, the only differences made will be the data model, fields I fetch from the database, and their transformation to the datamodel and json response.

  1. $app->get('/animals', AnimalFindAction::class); calls App\Action\Animal\AnimalFindAction
  2. App\Action\Animal\AnimalFindAction::__construct() gets a AnimalFinder service.
  3. App\Action\Animal\AnimalFindAction::__invoke() calls App\Domain\Animal\Service\AnimalFinder::findAnimals().
  4. App\Domain\Animal\Service\AnimalFinder::__construct() gets a App\Domain\Animal\Repository\AnimalFinderRepository repo.
  5. App\Domain\Animal\Service\AnimalFinder::__findAnimals() calls App\Domain\Animal\Repository\AnimalFinderRepository::findAnimals().
  6. App\Domain\Animal\Repository\AnimalFinderRepository::construct() gets App\Factory\QueryFactory.

This way the files will be almost identical. What I would like to know is how to code the Action, Service and Domain classes best, without having this much identical code, but without breaking the ADR pattern.

odan commented 3 years ago

I don't see duplicate code in your example. All these new classes are specific to their uses case. They have different routes, different parameters, different validation, different mapping, different business logic, different queries, different filters, different tables, different HTTP responses etc. So it makes absolutely sense to put this kind of logic into separate classes and modules. Your example does perfectly match to the ADR pattern.

DigiLive commented 3 years ago

I think I do understand what you mean, having ADR in mind, but...

For example... 35/43 (80%) lines are identical between UserFinderRepository and AnimalFinderRepository. I'm looking for a way to avoid such a high ratio of duplicate code without breaking the ADR pattern.

final class UserFinderRepository // vs AnimalFinderRepository
{
    private QueryFactory $queryFactory;

    public function __construct(QueryFactory $queryFactory)
    {
        $this->queryFactory = $queryFactory;
    }

    public function findUsers(array $options): array // vs findAnimals(array $options): array
    {
        $query = $this->queryFactory->newSelect('users'); // vs $this->queryFactory->newSelect('animals');
        $query->select(['id', 'username']); // vs $query->select(['id', 'animal_name']);

        $order  = $options['order'] ?? 'users.id'; // vs $order  = $options['order'] ?? 'animals.id';
        $dir    = $options['dir'] ?? 'asc';
        $limit  = max($options['limit'] ?? 10, 10);
        $offset = min($options['offset'] ?? 0, 0);

        if ($order) {
            $dir === 'desc' ? $query->orderDesc($order) : $query->order($order);
        }

        if ($limit) {
            $query->limit((int)$limit);
        }

        $query->offset((int)$offset);

        return $this->toList($query->execute()->fetchAll('assoc') ?: []);
    }

    private function toList(array $items): array
    {
        $users = []; // vs $animals

        foreach ($items as $data) {
            $users[] = new UserData($data); // vs $animals[] = new AnimalData($data);
        }

        return $users; // vs return $animals;
    }
}

My apologies for being confused and thank you very much for your efforts.

odan commented 3 years ago

Ok fine. Please note that the ADR pattern does not define how to structure / implement the "Domain" layer (the D in ADR). How to implement the Domain is up to the developer.

So your question is more about the query builder logic and how to reuse this specific part of this query. In a non CRUD application this part of code should not be required. But when your API is CRUD oriented you may need such a code quite often.

So I would extract this specific code into another class, for example into the class like QueryFactory.

Pseudo example code:

/**
 * Create a new 'select' query for the given table and add optional conditions.
 *
 * @param string $table The table name
 * @param array $params The query params
 *
 * @return Query A new select query
 */
public function newSelectWithConditions(string $table, array $params): Query
{
    $query = $this->newSelect($table);

    $order = $params['order'] ?? 'users.id';
    $dir = $params['dir'] ?? 'asc';
    $limit = max($params['limit'] ?? 10, 10);
    $offset = max($params['offset'] ?? 0, 0);

    if ($order) {
        $dir === 'desc' ? $query->orderDesc($order) : $query->order($order);
    }

    if ($limit) {
        $query->limit((int)$limit);
    }

    $query->offset((int)$offset);

    return $query;
}

You could also just change the existing newSelect method with the code from above.

Repository Usage

$query = $this->queryFactory->newSelectWithConditions('users', $params);

$query->select(
    [
        'id',
        'username',
        'first_name',
        'last_name',
        // ...
    ]
);

return $this->toList($query->execute()->fetchAll('assoc') ?: []);

The toList method cannot be moved to another central place, because PHP does not support generics yet.

To hydrate an array of objects in a more generic way you may try to use the laminas/hydrator:

Examples:

The laminas/hydrator does basically something like this:

/**
 * Convert to list of objects.
 *
 * @param array $items The items
 * @param string $class The FQN
 *
 * @return array The list
 */
public function toList(array $items, string $class): array
{
    $users = [];

    foreach ($items as $data) {
        $users[] = new $class($data);
    }

    return $users;
}

Usage:


// ...

$rows = $query->execute()->fetchAll('assoc') ?: [];

return $this->collection->toList($rows, UserData::class);
odan commented 3 years ago

I just added an example to this skeleton project. See here.