-
Notifications
You must be signed in to change notification settings - Fork 0
given solution #1
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,65 @@ | ||
<?php | ||
|
||
namespace src\Decorator; | ||
|
||
use DateTime; | ||
use Exception; | ||
use Psr\Cache\CacheItemPoolInterface; | ||
use Psr\Log\LoggerInterface; | ||
use src\Integration\DataProvider; | ||
|
||
class DecoratorManager extends DataProvider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Плохое название класса. По сути, это DataProvider с возможностью использования кэша. Скорее всего это какой-нибудь There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Лучше использовать не наследование, а композицию. Увеличивает переиспользуемость кода |
||
{ | ||
public $cache; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. нет phpDoc для свойств область видимости свойства объекта должна быть аналогично для свойства $logger - у него уже есть сеттер |
||
public $logger; | ||
|
||
/** | ||
* @param string $host | ||
* @param string $user | ||
* @param string $password | ||
* @param CacheItemPoolInterface $cache | ||
*/ | ||
public function __construct($host, $user, $password, CacheItemPoolInterface $cache) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
В случае с использованием композиции. конструктор будет выглядеть так: |
||
{ | ||
parent::__construct($host, $user, $password); | ||
$this->cache = $cache; | ||
} | ||
|
||
public function setLogger(LoggerInterface $logger) | ||
{ | ||
$this->logger = $logger; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function getResponse(array $input) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. некорректный phpDoc (в родителе DataProvider нет метода getResponse) или ошибка в названии метода и аргумента - должно быть |
||
{ | ||
try { | ||
$cacheKey = $this->getCacheKey($input); | ||
$cacheItem = $this->cache->getItem($cacheKey); | ||
if ($cacheItem->isHit()) { | ||
return $cacheItem->get(); | ||
} | ||
|
||
$result = parent::get($input); | ||
|
||
$cacheItem | ||
->set($result) | ||
->expiresAt( | ||
(new DateTime())->modify('+1 day') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Срок действия кэша лучше задавать аргументом в конструктор. для увеличения переиспользованности класса |
||
); | ||
|
||
return $result; | ||
} catch (Exception $e) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Желательно обрабатывать ошибки по интерфейсу \Throwable |
||
$this->logger->critical('Error'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. если logger не обязательное свойство объекта. то должна быть проверка There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Не информативное сообщение лога There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. некорректный уровень логирования - скорее всего должно быть |
||
} | ||
|
||
return []; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Некорректно в случае возникновения ошибки возвращать пустой массив. Во-первых, это может быть не тот тип данных, который ожидается клиентом; во-вторых, логика клиента при пустом ответе и при возникновении ошибки, скорее всего, должна различаться |
||
} | ||
|
||
public function getCacheKey(array $input) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. отсутствует phpDoc метода There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. область видимости метода должена быть |
||
{ | ||
return json_encode($input); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. необходимо предусмотреть вероятность изменения порядка ключей в массиве, так как для двух одинаковых по логике массивов [
'key1' => 'value1',
'key2' => 'value2',
]
[
'key2' => 'value2',
'key1' => 'value1',
] метод будет отдавать разные ключи и не будет использовать кэш There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. встроенные функции необходимо вызывать через глобальное пространство имён для повышения производительности https://veewee.github.io/blog/optimizing-php-performance-by-fq-function-calls/ |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
<?php | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. не хватает объявления declare(strict_types=1); для строгой проверки входящих параметров функций |
||
namespace src\Integration; | ||
|
||
class DataProvider | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. лучше создать интерфейс, на который можно было бы завязывать другие сервисы |
||
{ | ||
private $host; | ||
private $user; | ||
private $password; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. нет phpDoc для свойств |
||
|
||
/** | ||
* @param $host | ||
* @param $user | ||
* @param $password | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Нет описания типов аргументов |
||
*/ | ||
public function __construct($host, $user, $password) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. не типизированы аргументы |
||
{ | ||
$this->host = $host; | ||
$this->user = $user; | ||
$this->password = $password; | ||
} | ||
|
||
/** | ||
* @param array $request | ||
* | ||
* @return array | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Если есть возможность. необходимо описать какие типы данных или какую структуру имеет возвращаемый массив |
||
*/ | ||
public function get(array $request) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. нет типизации для возвращаемого значения public function get(array $request): array |
||
{ | ||
// returns a response from external service | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. тут точно не должно быть реализации? |
||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не хватает объявления
declare(strict_types=1);
для строгой проверки входящих параметров функций