2
\$\begingroup\$

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.

\$\endgroup\$
4
  • \$\begingroup\$ I have an Offers class which needs a method isLawyer — I'm having a hard time understanding what you are trying to accomplish here. What does an "Offers" object represent, and why would it be a lawyer or not? The whole situation makes no sense to me; perhaps some more background information would help. \$\endgroup\$ Commented Jun 20, 2017 at 19:07
  • \$\begingroup\$ Thanks for your interest, I edited the question adding context. \$\endgroup\$ Commented Jun 20, 2017 at 19:16
  • \$\begingroup\$ Are there other categories of offers, or is "lawyer" the only one? \$\endgroup\$ Commented Jun 20, 2017 at 19:18
  • \$\begingroup\$ There are dozens of categories yep. \$\endgroup\$ Commented Jun 20, 2017 at 19:20

1 Answer 1

2
\$\begingroup\$

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?

\$\endgroup\$
2
  • \$\begingroup\$ Thanks for your interest. I'd love to answer all of this, should I create a new answer or edit yours and add answers to it ? \$\endgroup\$ Commented Jun 21, 2017 at 10:16
  • \$\begingroup\$ @SteveChamaillard Most of my questions were more hypothetical in nature - hopefully to help prompt some rethinking for how you have structured your classes. I think you are rightfully concerned about how your objects are interacting and that mainly that concern is based in a current design that does not map well to real-world concepts. \$\endgroup\$
    – Mike Brant
    Commented Jun 21, 2017 at 14:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.