I
I
Ilya2020-02-10 15:02:12
PHP
Ilya, 2020-02-10 15:02:12

What is the best way to handle method parameters?

I had an argument with a colleague about class design.

There is a class for calculating the delivery of goods to a certain city by two delivery services: sdek and ems (to simplify, only two. There may be more).

The class has two dependencies, the sdek calculator and the ems calculator. How they work in this case is not important.

There is a public calculate method that takes two parameters: an array of goods and a city.

There are also several internal methods that are responsible for calculating specific services cdekCourier cdekPickup emsPickup.
Each of these methods requires the same products and city to calculate.

Dispute essence:
One considers that in each method (cdekCourier cdekPickup emsPickup) it is necessary to transfer parameters.
Another believes that it is better to write the goods and the city into properties, and then access them from these methods.

We cannot argue with each other.

The class itself in two versions is shown below.

The question is which option is better and why? Or are they both bad?

Option 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;
  }
}

Option 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;
  }
}

Answer the question

In order to leave comments, you need to log in

2 answer(s)
X
xfg, 2020-02-11
@xfg

It is quite simple to argue and even refer to specific paragraphs from the Implementing domain-driven design book, namely, what the author writes about services. The author of the book explains quite clearly why services should not have state.
You have a classic service and you have a problem, just because you basically approached the design of your service the wrong way.
First of all you need to design the interface CalculatorInterface

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

Next you need to create two implementations of this interface CdekCalculator and EmsCalculator
class CdekCalculator implements CalculatorInterface {
  public function calculate($products, $city) {
     // реализация расчета через cdek
  }
}

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

And now your shipping class will look like this
class Delivery {
  private $calculator;

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

There is no more contention. You simply pass the desired implementation of the calculator to the Delivery class. The code above is a pseudo language, not php, and generally just a basic idea of ​​how it should be. You will probably immediately have a question, how do you then display all the delivery calculations in your UI. But I'm sorry I can't write the whole application for you.
These are basic things in object-oriented programming. It's called subtype polymorphism. Without this knowledge, it is impossible to write good object-oriented code. Better both come to the conclusion that you urgently need to read some books about object-oriented programming and start with the very basics, what is polymorphism and in particular subtype polymorphism.
In general, protected/private methods and branching from conditional constructs are almost always an indicator of spaghetti code and the first sign that you write procedural code using OOP syntax. You will come to this conclusion if you develop with TDD, and then you can google your assumptions from other programmers.

I
Ivan Yakushenko, 2020-02-10
@kshnkvn

Uncle Bob used to say "The best functions are those with no parameters!". So properties.

Didn't find what you were looking for?

Ask your question

Ask a Question

731 491 924 answers to any question