3
\$\begingroup\$

I want to abstract some code which does a bunch of checks and returns a boolean - since I am using the business logic in several places. But I also want to get back the reason why, if it was returned FALSE. I have this in mind...

class ContactCanBeContactedCheck
{
    public $not_contactable_reason;

    const REASON_DECEASED = 0;
    const REASON_OPTED_OUT = 1;

    public function __invoke(Contact $contact)
    {
        // Deceased
        if ($contact->is_deceased) {
            $this->not_contactable_reason = static::REASON_DECEASED;
            return FALSE;
        }

        // Opted out
        if ($contact->opt_out) {
            $this->not_contactable_reason = static::REASON_OPTED_OUT;
            return FALSE;
        }

        return TRUE;
    }
}

I would then call the code like so...

$contact_can_be_contacted_check = new ContactCanBeContactedCheck;
$contact_can_be_contacted_check($contact);
$reason = $contact_can_be_contacted_check->not_contactable_reason;

I think this must be a well-trodden path: needing a decision, plus a reason for said decision. Is this a suitable approach or is there another more widely accepted?

\$\endgroup\$
4
\$\begingroup\$

I want to abstract some code...

What is it that you actually want to abstract?

Unless you are going to pass an instance of ContactCanBeContactedCheck class to someone who expects a callable, you shouldn't implement it through __invoke method.

My first though was, why not make it a method of the Contact class?

class Contact
{
  public const NOT_CONTACTABLE_REASON_DECEASED = 1;
  public const NOT_CONTACTABLE_REASON_OPTED_OUT = 2;

  // ...

  public function getNotContactableReason(): ?int
  {
        if ($this->is_deceased) {
            return static::NOT_CONTACTABLE_REASON_DECEASED;
        }

        if ($this->opt_out) {
            return static::NOT_CONTACTABLE_REASON_OPTED_OUT;
        }

        return null;
  }
}

Now it is contained in the class that actualy has the data to tell the reason. Encapsulation is one of the fundamental principles of OOP.

Still, if you need a callable, you can now do just

$callback = fn (Contact $contact): ?int => $contact->getNotContactableReason();

Also your method is trying to do 2 things

  • tell if the contact is contactable
  • tell the reason why it is not contactable

My method only tells the reason why it is not contactable, and null if there is no reason, because the contact is contactable. Using this approach you are actually able to tell both those things using just this one method as well. But it is not intent to do two things like in your method. It's just a side effect. And that makes a big difference.

You could add another method that would just tell whether it is contactable regardless of reason, but it is now easy enough to tell from the first method as well, so the second method would be just sugar for $this->getNotContactableReason() === null.

Notice I started the constants at 1, not 0. Zero is falsey, just like null. And using zero could then easily lead to mistakes if you use weak comparision.

if ($reason) {...}

vs.

if ($reason !== null) {...}

I am wondering though, what you are actually doing with the class and the resulting reason. If the reason ends up in a switch or an if, then the entire method is useless and you should branch your code based directly on the $is_deceased and $opt_out properties.

Those 3 lines of usage are far from enough to tell what you really need. If I had more, I could target my review much better.

\$\endgroup\$
1
  • \$\begingroup\$ Yeah, nice - thanks. The abstraction is simply due to the fact that two independant parts of the app need to know if a Contact is 'contactable' - so all the logic within this new class I've thought up, are taking inline logic to a single place. I did want to keep this on the main Contact model, and now we've dropped the need for that $not_contactable_reason state variable, this now works better. Although, I was partly taking it out of the Contact model due to it being such a big thing in itself. But agree with you on the idea of it being part of the main Contact model. Thanks! \$\endgroup\$ – benjaminhull Jun 18 '20 at 17:13
3
\$\begingroup\$

In this case, quite often, the answer is to create some type of "enumeration" where one value means "success" and the rest are reasons for failure.

In your example, just a minor change:

class ContactCanBeContactedCheck
{
    const SUCCESS = 0;
    const FAILURE_REASON_DECEASED = 1;
    const FAILURE_REASON_OPTED_OUT = 2;

    public function __invoke(Contact $contact)
    {
        // Deceased
        if ($contact->is_deceased) {
            return static::FAILURE_REASON_DECEASED;
        }

        // Opted out
        if ($contact->opt_out) {
            return static::FAILURE_REASON_OPTED_OUT;
        }

        return static::SUCCESS;
    }
}

Of course, now, you can make __invoke a static function, because all of the decision logic is within.

\$\endgroup\$
1
  • \$\begingroup\$ Great - thanks. This is a much better approach and removes the slightly accident-prone $not_contactable_reason state variable. \$\endgroup\$ – benjaminhull Jun 18 '20 at 17:16

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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