Skyeng interview test

ApiClient.php

<?php

namespace src\Integration;

use src\Contracts\ApiRequestInterface;
use src\Contracts\ApiResponseInterface;
use src\Contracts\ApiClientInterface;
use Psr\Http\Client\ClientInterface;

/**
 * Class ApiClient
 *   Используется для взаимодействия с REST API сервиса XYZ
 *   ...
 */
class ApiClient implements ApiClientInterface
{
    /** @var ClientInterface */
    private $httpClient;

    /**
     * @param ClientInterface $httpClient
     */
    public function __construct(ClientInterface $httpClient)
    {
        $this->httpClient = $httpClient;
    }

    /**
     * Выполняет запрос к АПИ получая какие-то данные
     * 
     * @param ApiRequestInterface $request
     *
     * @return ApiResponseInterface
     */
    public function fetchData(ApiRequestInterface $request)
    {
        // $this->httpClient->sendRequest(...)
        // returns a response from external service
    }
}

ApiClientInterface.php

<?php

namespace src\Contracts;

use ApiRequestInterface;
use ApiResponseInterface;

/**
 * Интерфейс для реализации паттерна декоратор
 */
interface ApiClientInterface
{
    /**
     * @param ApiRequestInterface $request
     * @return ApiResponseInterface
     */
    public function fetchData(ApiRequestInterface $request);
}

ApiRequestInterface.php

<?php

namespace src\Contracts;

/**
 * Интерфейс для формирования запроса к АПИ XYZ
 */
interface ApiRequestInterface
{

}

ApiResponseInterface.php

<?php

namespace src\Contracts;

/**
 * Интерфейс ответа АПИ XYZ
 */
interface ApiResponseInterface
{

}

CachableApiDataManager.php

<?php

namespace src\Decorator;

use Exception;
use Psr\Cache\CacheItemPoolInterface;
use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerAwareTrait;
use src\Integration\ApiClient;
use src\Factory\CacheKeyFactory;
use src\Contracts\ApiRequestInterface;
use src\Contracts\ApiResponseInterface;
use src\Contracts\ApiClientInterface;

/**
 * Class CachableApiClient
 *   Обертка над запросами к АПИ с кешированием
 */
class CachableApiClient implements ApiClientInterface, LoggerAwareInterface
{
    use LoggerAwareTrait;

    /** @var CacheItemPoolInterface */
    protected $cache;

    /** @var ApiClientInterface */
    protected $apiClient;

    /** @var CacheKeyFactory */
    protected $cacheKeyFactory;

    /** @var int Время в секундах */
    protected $cacheTtl;

    /**
     * @param ApiClientInterface $apiClient
     * @param CacheItemPoolInterface $cache
     * @param CacheKeyFactory $cacheKeyFactory
     * @param int $cacheTtl
     */
    public function __construct(
        ApiClientInterface $apiClient, 
        CacheItemPoolInterface $cache, 
        CacheKeyFactory $cacheKeyFactory,
        $cacheTtl
    ) {
        $this->apiClient = $apiClient;
        $this->cache = $cache;
        $this->cacheKeyFactory = $cacheKeyFactory;
        $this->cacheTtl = $cacheTtl;
    }

    /**
     * @param ApiRequestInterface $request
     * 
     * @return ApiResponseInterface
     */
    public function fetchData(ApiRequestInterface $request)
    {
        $cacheKey = $this->cacheKeyFactory->createRequestKey($request);

        try {
            $cacheItem = $this->cache->getItem($cacheKey);
            if ($cacheItem->isHit()) {
                return $cacheItem->get();
            }

            $result = $this->apiClient->fetchData($request);

            $cacheItem
                ->set($result)
                ->expiresAfter($this->cacheTtl);

            $this->cache->save($cacheItem);

            return $result;
        } catch (Exception $e) {
            $this->logger->critical('Failed to fetch data from API', ['message' => $e->getMessage()]);

            throw $e;
        }
        // catch(RequestExceptionInterface $e)
    }
}

CacheKeyFactory.php

<?php

namespace src\Factory;

use src\Contracts\ApiRequestInterface;

/**
 * Фабрика для формирования ключей кеша
 */
class CacheKeyFactory
{
    /**
     * @param ApiRequestInterface $request
     * 
     * @return string
     */
    public function createRequestKey(ApiRequestInterface $request) 
    {
        return hash(...);
    }
}

SkyEng_test_review.md

Тестовое задание в SkyEng на PHP разработчика. SkyEng test review for PHP developer.

• Делаю допущение, что код написан под версию php 5, поэтому не оставляю комментарии на счет пропущенных указаний типов, сопутствующих версии 7
• Исхожу из того, что класс DataProvider независимая сущность, которая может использоваться напрямую клиентским кодом, без слоя кеширования, а значит не требует объявления abstract и закрытия метода get() в доступ protected (об этом в пункте 8)
• Пропускаю любые замечания по поводу тестов (их, очевидно, нет)
• Если код работает с каким-то конкретным АПИ, то вероятно, его адрес это константа, но допустим, что инстанс класса конфигурируется каким-то DI контейнером, который заполнит значение поля $host из переменных окружения
• Мне не до конца ясна абстракция примера, поэтому я не стану делать замечание про $host $user и $password, а просто исправлю это в своей реализации на использование интерфейса Psr\Http\Client\ClientInterface
• В условии написано, что разработчик справился с задачей, но кажется в коде забыто сохранение результата в кеш пул $this->cache->save($cacheItem);
• Is it ok to use Russian language in comments? :)

Ревью:

1. Пропущен phpdoc для класса с пояснениями об обязаностях класса class DataProvider

2. class DataProvider назван неочевидно. Задача класса, взаимодействовать с АПИ, никак не отображена в его названии

3. Для свойств класса DataProvider необходимо указать тип в phpdoc. Это поможет тем, кто будет наследоваться от этого класса — IDE укажет предполагаемый тип переменных класса

4. Тоже самое для конструктора класса DataProvider. В phpdoc пропущены типы аргументов

5. Метод public function get(array $request) класса DataProvider - название выбрано неудачно. Нет ясности из сигнатуры метода, что именно получает метод (и что возвращает по phpdoc)

6. Из условий задачи и реализации метода get не совсем понятно как именно используются классы и какие параметры принимют для выполнения запроса, но кажется у public function get(array $request) входным аргументом должен быть интерфейс RequestInterface, а не массив неопределенных данных. Тоже самое касается возвращаемого типа. Может это ResponseInterface?

7. Класс class DecoratorManager extends DataProvider. Теже пункты - phpdoc и название. Допустим phpdoc можно опустить, но из названия не ясно, что делает класс, кроме того, что декорирует что-то

8. Исходя из названия DecoratorManager можно предположить, что класс задумывался как реализация паттерна декоратор, а значит стоит применить композицию, а не наследование от провайдера АПИ DataProvider, что также позволит покрыть обе сущности собственными, независимыми тестами

9. В классе class DecoratorManager extends DataProvider явное нарушение SRP: формирование ключа для кеша можно вынести в отдельную фабрику

10. Не вижу ни единой причины свойствам $cache и $logger у класса DecoratorManager быть публичными. Так же пропущен phpdoc для обоих свойств

11. Ну раз уж есть public function setLogger(LoggerInterface $logger) то можно сообщить контейнеру (или просто клиентскому коду), который будет инжектировать через этот сеттер зависимость, что класс DecoratorManager имеет эту зависимость и указать реализацию интерфейса Psr\Log\LoggerAwareInterface. Там же где-то и трейт есть LoggerAwareTrait

12. public function getResponse(array $input) как и у родительского класса.. может тут что-то вроде makeApiRequest или fetchDataFromApi?

13. Аналогичные вопросы к входным и возвращаемым типам у метода public function getResponse(array $input). Опять же, из задачи не совсем очевидно, но думаю в реальной жизни можно сделать более очевидную сигнатуру с явными типами для исключения ошибок, связанных с массивом: public function getResponse(ApiRequestInterface $request): ApiResponseDataInterface

14. (new DateTime())->modify('+1 day') время жизни кеша стоит вынести в константу (или в параметр, который можно передать при создании объекта)

15. $this->logger->critical('Error'); ну можно еще передать $e->getMessage() для большей наглядности и сообщение изменить на какое-то явное, вроде Failed to fetch data from API

16. } catch (Exception $e) { отлавливать корневой эксепшен не очень хорошая практика, но возможно это допустимое решение для данной задачи. Как вариант - разбить на несколько блоков catch с разным поведением и разным уровнем логирования. Например, ошибки связанные с сетью, пробрасывать выше, чтобы клиентский код сам решал как на них реагировать

17. public function getCacheKey(array $input) если этот метод, после рефакторинга, остается в классе, то у него нет причин быть публичным

18. return json_encode($input); можно, но ненадежно. При вызове getCacheKey нужно помнить что json_encode может вернуть false в случае ошибки. Используя фабрику из пункта 9, стоит сделать формирование ключа более надежным, например пропустив какой-то ключевой элемент массива через функции хеширования