Setter use for dependency injection, passing interface through too many layers

Question

I have an Offers class which is basically a seller which has specific skills, such as law knowledge. It needs a method isLawyer. Offers has a parent_cat_id column, which references the id of a Category.

Here's Offers :

<?php

namespace App\Models;

use App\Services\CategoryService\AbstractCategory;
use LogicException;

class Offers
{
    /** @var AbstractCategory $category */
    private $categoryService;

    public function isLawyer(): bool
    {
        return $this->getCategoryService()->isLawyer($this->getParentCatId());
    }

    public function setParentCatId(int $catId)
    {
        $this->parent_cat_id = $catId;
    }

    public function getParentCatId(): int
    {
        return (int)$this->parent_cat_id;
    }

    public function setCategoryService(AbstractCategory $categoryService)
    {
        $this->categoryService = $categoryService;
    }

    public function getCategoryService(): AbstractCategory
    {
        if (is_null($this->categoryService)) {
            throw new LogicException('Category service should be set beforehand.');
        }

        return $this->categoryService;
    }
}

AbstractCategory is an abstract class that different categories will extend. I don't care about the implementation of this as of now.

Now in tests, it's all good and fun :

/** @test */
public function is_lawyer_defers_to_category_is_lawyer_method()
{
    $category = Mockery::mock(AbstractCategory::class);
    $category->shouldReceive('isLawyer')->once()->with(1)->andReturn(true);

    $offer = new Offers;
    $offer->setParentCatId(1);
    $offer->setCategoryService($category);

    $this->assertTrue($offer->isLawyer());
}

But as for the real implementation, it gets tricky, as I have 3-4 layers before I'm using $offers->isLawyer().

<?php

namespace App\Models\Product\Chat;

use App\Models\Offers;

class Chat
{
    /** @var Offers $offer */
    private $offer;

    /**
     * @param Offers $offers
     */
    public function setOffer(Offers $offers)
    {
        $this->offer = $offers;
    }

    /**
     * @return Offers
     */
    public function getOffer()
    {
        return $this->offer;
    }

    /**
     * @return bool
     */
    public function byLawyer(): bool
    {
        return $this->getOffer()->isLawyer();
    }
}

Chat uses isLawyer of Offers, but byLawyer is itself called by something else.

I still need to set the AbstractCategory before I use this method.

So, should I pass it through as a parameter to every step ? This sounds ridiculous.

Should I "setup" Offers and set the AbstractCategory even before it's assigned to the Chat ?

Should I give up and instantiate a new Category right in the Offers class ? Setting the abstraction felt like a better design.

Thanks.

Answer 1

Your class names are really throwing me off. Perhaps because they are so generically named. By looking at these classes I have a hard time understanding the interfaces you are trying to expose.

What is a Chat? Why does a chat hold references to Offers (which seems to only be a single "offer" so should perhaps be named Offer)? Why would a Chat object need a method to check if the Offers it holds is byLawyer (which perhaps should be named isLawyer() since it returns boolean.)?

I just honestly can't make heads or tails of it because I don't think your classes are defined upon real-world lines. Make sure you name your classes meaningfully and specifically, and most importantly with relevance to real-word concepts. 9 times out of 10, if you find yourself thinking of you objects in real-world terms, you will come up with a better design. In the real world, does a chat have the ability to determine if it has something to do with a lawyer? No, a chat may need to know about the parties chatting, have some send/receive message functionality, maybe store messaging history, etc. What does it have to do with lawyers or offers?

What is a CategoryService (ServiceCategory?) and what does it have to do with lawyers (are lawyer services one category of service?)

---

Lots of questions around Offers:

• Why would an Offer(s) object hold information on categories? Wouldn't this be an opposite relationship in real world terms (where categories hold offers)?
• If you require AbstractCategory (or CategoryService) as a dependency for Offers why not pass that dependency in constructor? Why let your class be set up in a bad state?
• Do you really need Offers to be mutable, or will it always have a 1-to-1 relationship with CategoryService it wraps? If so, why the need for the getters/setters?
• Does Offers even provide any meaningful functionality above and beyord the AbstractCategory object it holds.
• Should AbstractCategory truly be an abstract class or an interface? Usually when you pass a generalized object like this, you are depending on that object having a given interface, not that it inherits from some class. I would seem a bit odd to be type hinting against an abstract class.

---

$this->parent_cat_id

You don't have parent_cat_id defined as property on this class.

---

return (int)$this->parent_cat_id;

There should be no reason for type casting here.

---

Chat can be set up in a bad state. byLawyer() method can perform unexpectedly if offer property has not been set. Should Offers be passed as dependency on constructor?

---

Why inconsistent use of doc blocks?

---

Don't mix camelCase and snake_case in your own code.

---

Why inconsistent use of return type hinting?