Send an inbound Enquiry to a Users registered Twitter Account to notify them of its arrival

Question

This follows on from an earlier CR Question. - The reasoning behind which suggestions were taken on / which were not taken on board are no longer relevant as this is supposed to be, from my understanding, a distinct question that is capable of standing alone should anyone come across it.

Background

An enquiry is posted from a "Contact Me widget" on a registered user's 3rd party domain to a processing queue, this is one of the jobs that is triggered via an event handler that batch processes the queue and spawns events containing any new Enquiry Identifiers since the last time the queue was processed.

<?php

namespace App\Listeners\Enquiry;

use App\Events\NewEnquiryEvent;
use App\Models\Stores\Enquiry;

class TweetNewEnquiry
{
    /**
     * On enquiry event send tweet notifying the Enquiry target,
     * if applicable.
     * 
     * @param NewEnquiryEvent
     *   The new enquiry event object.
     */
    public function handle(NewEnquiryEvent $event) : void
    {
        $enquiry_ids = (array) $event->body->id;

        $enquiries_to_tweet = Enquiry::find($enquiry_ids)->filter(function($enquiry) {
            return $enquiry->canTweet() && $enquiry->shouldTweet();
        });

        $enquiries_to_tweet->each->tweet();
    }
}

Answer 1

Context:

This answer was originally supplied in context as part of a larger answer, when the question was split here I've also split the answer here

Updated code

The updated code does create a better signal to the reader of what is going on, I particularly like the variable name coming out one of the operations. You are trying to write Laravel code so I'll skip over framework coupling issues

Had a look at Laravel's docs and there is a filter method on the collection class so we could change the code to this:

filter(function (Enquiry $enquiry) : bool {
  return $enquiry->canTweet() || $enquiry->shouldTweet();
));

The reason to inverse the check from reject to filter is that I find it hard personally to read negative code, With !$something, first your brain has to parse what $something is, and then it has to flip it, I would advise as much as possible to write code that talks about the situation from the "positive" prospective.

I've also added type hinting again, seems like you agree that type hinting is overall good but just not in the habit of doing so? Remember anonymous functions can be type hinted too.

I would also suggest moving that anonymous function out into it's own function not necessarily just for code reuse (although that's a nice side effect) but because it forces you to create a semantic name for what the combination of canTweet() and shouldTweet() is.

I definitely appreciate it doesn't look necessary now to create a new function but if you default to doing so you encourage other developers to not lump all their changes at this particular call point (good if this eventually gets reused elsewhere).

Also you tell them the intent of the code so hopefully they won't dump unrelated stuff into your function, code always grows in active projects, decisions you make now will affect future code more than you know, variable names/function names that you decide now will be replicated in other parts of the code and particularly classes they get sticky to everything because they are a natural clumping mechanism

Alternatively (or additionally) I personally often assign the result of a condition to variable before return to force myself to name it and it also helps when using xdebug, because you can then see the result of the return inside the function itself, this makes stepping over/debugging easier because xdebug cannot go in reverse see: https://bugs.xdebug.org/view.php?id=888 :(

We can also stop our coupling to the structure of NewEnquiryEvent by changing the signature of the handle method to array $enquiry_ids

Feels like the OOP way to do this would be to create a method that does this:

class NewEnquiryEvent 
{
  public function getIds() : array 
  {
    return $this->body->id;
  }
}

Personally I would prefer to do this:

namespace NewEnquiryEvent; 
function getIds(NewEnquiryEvent $event) : array 
{
  return $event->body->id;
}

This is because I prefer to take responsibly away from classes they're usually already doing too much

I find OOP developers typically set things to private or protected for one of these reasons:

• They think it makes the property immutable
• They think know better than the callee about how the object should be used, and they think they can dictate that
• They do it because they were taught to do so by default without really questioning why

Let's assume that's happened and we can't edit their class because we don't want to debug their class or its a third party code that we want are not interested in maintaining our own fork of then use the following:

namespace NewEnquriyEvent;
function getIds(NewEnquiryEvent $event) : array 
{
  return (function () : array {
    return $this->body->id;
  })->call($event);
}

I imagine this won't be consistent with your current project structure or in particular your autoloader and sometimes consistency should trump logic or programming style, but either way I think it's interesting to explain the train of thought.