Как лучше обращаться с параметрами метода?

Question

[Илья]

С коллегой возник спор на тему проектирования классов.

Есть класс для расчёта доставки товаров в определённый город двумя службами доставки: сдэк и емс (для упрощения только две. Их может быть больше).

у класса есть две зависимости, калькулятор сдэка и калькулятор емс. Как они работают в данном случае не важно.

Есть публичный метод calculate, который принимает два параметра: массив товаров и город.

Так же есть несколько внутренних методов, которые отвечают за расчёт по конкретным службам cdekCourier cdekPickup emsPickup.
Каждому из этих методов требуются для расчёта те же товары и город.

Суть спора:
Один считает, что в каждый метод (cdekCourier cdekPickup emsPickup) нужно передавать параметры.
Другой считает, что товары и город лушче записать в свойства, а потом обращаться к ним из этих методов.

Аргументировать друг другу свой вариант не можем.

Сам класс в двух вариантах приведён ниже.

Вопрос, какой вариант лучше и почему? Или может они оба плохи?)

Вариант 1

class DeliveryCalculator1
{
	protected $cdekCalculator;
	protected $emsCalculator;

	public function __construct($cdekCalculator, $emsCalculator)
	{
		$this->cdekCalculator = $cdekCalculator;
		$this->emsCalculator = $emsCalculator;
	}

	public function calculate($products, $city): array
	{
		$conditions = [];

		if ($condition = $this->cdekCourier($products, $city)) {
			$conditions[] = $condition;
		}

		if ($condition = $this->cdekPickup($products, $city)) {
			$conditions[] = $condition;
		}

		if ($condition = $this->emsPickup($products, $city)) {
			$conditions[] = $condition;
		}

		return $conditions;
	}

	protected function cdekCourier($products, $city): array
	{
		//тут ещё какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->cdekCalculator->courier($products, $city);

		return $condition ?: null;
	}

	protected function cdekPickup($products, $city): array
	{
		//тут ещё какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->cdekCalculator->pickup($products, $city);

		return $condition ?: null;
	}

	protected function emsPickup($products, $city): array
	{
		//тут ещё какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->emsCalculator->pickup($products, $city);

		return $condition ?: null;
	}
}

Вариант 2

class DeliveryCalculator2
{
	protected $cdekCalculator;
	protected $emsCalculator;

	protected $products;
	protected $city;

	public function __construct($cdekCalculator, $emsCalculator)
	{
		$this->cdekCalculator = $cdekCalculator;
		$this->emsCalculator = $emsCalculator;
	}

	public function calculate($products, $city): array
	{
		$this->products = $products;
		$this->city = $city;

		$conditions = [];

		if ($condition = $this->cdekCourier()) {
			$conditions[] = $condition;
		}

		if ($condition = $this->cdekPickup()) {
			$conditions[] = $condition;
		}

		if ($condition = $this->emsPickup()) {
			$conditions[] = $condition;
		}

		return $conditions;
	}

	protected function cdekCourier(): array
	{
		//тут еще какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->cdekCalculator->courier($this->products, $this->city);

		return $condition ?: null;
	}

	protected function cdekPickup(): array
	{
		//тут еще какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->cdekCalculator->pickup($this->products, $this->city);

		return $condition ?: null;
	}

	protected function emsPickup(): array
	{
		//тут еще какая-то логика, которая определяет условия доставки, если она вообще возможна
		$condition = $this->emsCalculator->courier($this->products, $this->city);

		return $condition ?: null;
	}
}

Comments

[Евгений Ромашкан]

Лучше тот вариант, при котором будет меньше связность и проще для клиентского кода. Может быть вообще без разницы.

Или может они оба плохи?)

Они оба очень плохи отсутствием типов и protected полями, лучше этим займитесь)

[Илья]

Евгений Ромашкан

отсутствием типов

Это просто упрощенный пример.

protected полями

Можно по-подробнее, пожалуйста?

[Евгений Ромашкан]

New_Horizons,

protected полями

Можно по-подробнее, пожалуйста?

Protected не нужно потому что:
1. Позволяет менять значения свойств в классе из наследника, то есть нарушает инкапсуляцию.
2. Поощряет расширение наследование.

[Иван Родичев]

Евгений Ромашкан,
1. Как переопределение свойств в наследнике нарушает инкапсуляцию?
2. Почему поощрять наследование это плохо?

[Евгений Ромашкан]

Nozdri,

Как переопределение свойств в наследнике нарушает инкапсуляцию?

Так что родитель не может полагаться на валидность стейта если его может править кто-то извне.

Почему поощрять наследование это плохо?

Потому что наследование это плохо :)
Потому что выше связность, и лучше юзать композицию.

[Дмитрий]

https://refactoring.guru/ru/design-patterns/factor... как вариант

Answer 1

[xfg]

Аргументировать довольно просто и даже сослаться на конкретные абзацы из книги Implementing domain-driven design, а именно, то что автор пишет про сервисы. Автор книги довольно четко рассказывает почему сервисы не должны обладать состоянием.

У вас классический сервис и у вас проблема, только лишь потому, что вы впринципе неправильно подошли к проектированию вашего сервиса.

В первую очередь вам нужно спроектировать интерфейс CalculatorInterface

interface CalculatorInterface {
  public function calculate($products, $city);
}

Далее вам нужно создать две реализации этого интерфейса CdekCalculator и EmsCalculator

class CdekCalculator implements CalculatorInterface {
  public function calculate($products, $city) {
     // реализация расчета через cdek
  }
}

class EmsCalculator implements CalculatorInterface {
  public function calculate($products, $city) {
    // реализация расчета через ems
  }
}

И теперь ваш класс доставки будет выглядеть таким образом

class Delivery {
  private $calculator;

  public function __construct(CalculatorInterface $calculator) {
     $this->calculator = $calculator;
  }
  public function calculate($products, $city) {
    //тут ещё какая-то логика, которая определяет условия доставки, если она вообще возможна
    return $this->calculator->calculate($products, $city);
  }
}

Предмета спора больше нет. Вы просто передаете в класс Delivery нужную реализацию калькулятора. Код выше это псевдоязык, не php и вообще только базовая идея того, как это должно быть. У вас наверняка тут же возникнет вопрос, как вам тогда выводить все расчеты доставки в ваш UI. Но извините не могу написать за вас всё приложение.

Это базовые вещи в объектно-ориентированном программировании. Называется полиморфизм подтипов. Без этих знаний невозможно писать хороший объектно-ориентированный код. Лучше оба придите к выводу, что вам срочно нужно почитать какие-нибудь книги об объектно-ориентированном программировании и начать с самых основ, что такое полиморфизм и в частности полиморфизм подтипов.

Вообще protected/private методы и ветвления из условных конструкций - это практически всегда показатель спагетти-кода и первый звоночек, что вы пишете процедурный код используя ООП синтаксис. Вы придете к этому выводу если будете разрабатывать через TDD, а затем сможете нагуглить ваши предположения у других программистов.

Comments

[Илья]

Спасибо за развёрнутый ответ. Да, действительно, часто ловлю себя на мысли что мы именно пишем в ООП синтаксисе, а не в ООП. Очень много protected методов, от которых избавиться пока не хватает знаний и опыта.

Насчёт состояния понятно, а вот с примерами не очень.

Два калькулятора, реализуют один интерфейс, окей. Но в чём смысл класса Delivery? Нам нужен расчёт именно по нескольким службам сразу, а не по одной.

[Denis]

ЗЫ перенес с ответа тк хотел написать коммент

Пример хороший, но мне кажется автора не удовлетворит, я так понял что автор юзает эти класы в эстиматоре, где в корзине показывается список доступных шипингов и соответствующих квот.
И описанный адаптер может смутить топик стартера где скорее всего по его логике будет примерно так

<?php
function EstimateShipping($shippings, $products, $city) 
{
$responseForView = [];
foreach($shippings as $shipping) {
   if (!$shipping->allowed()) {
     continue;
   }
   $cost = (new Delivery ($shipping->getCalculator()))->calculate($products, $city);
   if ($cost === false) {
     continue;
   }
   $responseForView[] = ['id'=> $shipping->getId(), 'name'=> $shipping->getName(), 'cost' => $cost];
}
return $responseForView;
}

и может возникнуть вопрос как в анекдоте про верблюдов - "Зачем нам весь этот тюнинг в зоопарке"

[xfg]

Denis, в таком случае можно использовать паттерн composite. Базовая идея та же самая построенная на полиморфизме типов, как и практически все шаблоны проектирования.

Я лишь хотел донести мысль, что когда есть что-то одного и того же типа, но разной реализации - то это история про полиморфизм в любом случае. Шарики, ручки, доставки, кеши или логгеры - неважно. Важно лишь то, что мы должны отделить разные алгоритмы одного типа от клиента через интерфейс.

Answer 2

[Ivan Yakushenko]

Дядя Боб говорил "The best functions are those with no parameters!". Так что свойства.