Skip to content
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

Nova2 refactoring #62

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

VitalyZibulski
Copy link

Nova2 refactoring

```php
$np = new \LisDev\Delivery\NovaPoshtaApi2('Ваш_ключ_API_2.0');
$np = new \LisDev\Controllers\NovaPoshtaApi2('Ваш_ключ_API_2.0');
Copy link

Choose a reason for hiding this comment

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

Не совсем понимаю зачем менять неймспейс, если прошлый вполне отражал свое назначение, а вот Controllers может вводить в заблуждение


class Format
{
public string $format;
Copy link

Choose a reason for hiding this comment

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

Почему свойство публичное если у него есть сеттер и геттер?

*
* @return NovaPoshtaApi2
*/
public function setFormat($format): Format
Copy link

Choose a reason for hiding this comment

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

тайпхинтинг аргументов

$this->throwErrors = $throwErrors;
$this
->setKey($key)
->setLanguage($language)
Copy link

Choose a reason for hiding this comment

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

в этом классе разве есть такой метод?

*
* @see https://my.novaposhta.ua/settings/index#apikeys
*/
protected $key;
Copy link

Choose a reason for hiding this comment

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

пхп 8 позволяет типизировать свойства, чем советую активно пользоваться

class AddressService
{
private PreparationDataService $preparationDataService;
private NovaPoshtaApiClient $novaPoshtaApiClient;
Copy link

Choose a reason for hiding this comment

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

зависимости от реализаций

*/
public function findNearestWarehouse($searchStringArray)
{
$searchStringArray = (array) $searchStringArray;
Copy link

Choose a reason for hiding this comment

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

зачем его приводить к массиву если можно указать в хинтиге что это массив

// Try to find current region
foreach ($areas as $key => $area) {
// Is current area found by string or by key
$found = $findByString
Copy link

Choose a reason for hiding this comment

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

А вот если бы это вынести в метод 'IsAreaFounded(...): bool` то и комментарий не нужен был бы

public function getArea($findByString = '', $ref = '')
{
// Load areas list from file
empty($this->areas) and $this->areas = (include dirname(__FILE__) . '/NovaPoshtaApi2Areas.php');
Copy link

Choose a reason for hiding this comment

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

жесткие зависимости и выполнение операций в проверках, плохая практика


class Model implements ModelInterface
{
protected Model $model;
Copy link

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