Code intent
The main problem with your code is that it does not give any reasons as to why it performs the way it does which makes understanding, reviewing or upgrading it difficult.
$event->body->id
What is a body id on the event? is it always present? I'm assuming its an int but I guess it could contain letters? Does $event_id
adequately describe it?
Enquiry::find(...)
Ok here I don't know the return signature of ::find()
I assume it finds N instances of Enquiry based on most framework behaviour, does it return an array or a object container? can I call it $enqueries
?
Next is the reject()
function which I assume is like array_filter
based on the name I guess it works in the inverse, checks that return true will be rejected? I have vague prior knowledge on this thanks to mind share of the chosen framework but if I had no knowledge I would assume it's your own function which would mean I need to read the implementation
Next we've got a nameless function that performs: !$enquiry->shouldTweet()
there's no type hinting so I don't know if reject()
is expecting a boolean at this point in time I'll have to read all of the function, my main question is what is an $enquiry
and what does it mean when it does not "shouldTweet()".
By chance you've used a boolean operator on the return so I can now confirm a boolean return feeding back to reject()
can we remove all doubt with a type hint?
Can we array_filter
on $enqueries
instead of reject()
? Then we can talk about shouldTweet()
without using !
.
Let's assume based on your implementation details thus far that there are multiple enquiry objects that are being filtered on whether they should tweet.
Is canTweet()
usable here? shouldTweet()
implies there might be mitigating circumstances to whether we can or cannot tweet.
The last call you have is a custom map()
function, a map function will typically convert child array elements into a different child array element but your operation next suggests it does not aim to do that, I think it aims to create a side effect, that's more suitable for array_walk
than array_map
.
The semantics is important because the inbuilt functions tell you what happens in the loop without having to read the loop, particularly helpful when a loop is a non trivial amount of lines long.
I'm assuming that sendToTwitter()
returns NULL because I've seen no evidence of its use, it is returned to the mapping operation but the result of that is now lost in the fluent object as far as we're concerned from this call point, a type hint could have hinted at the truth.
Code Reuse
The other problem your code has is that the anonymous functions which by themselves cannot be reused, if we upgrade them to full functions we give the reader some context through the function name and we are then able to call the code from multiple different locations.
The function name should help tell future maintainers how much scope the function should be responsible for and telegraph what the function is supposed to do in context.
Strong binding to your chosen framework
Your code is strongly bound to your framework which means it will be harder to extract out of the framework in the future and potentially brittle every time the framework upgrades or any of its components upgrade.
It's tempting to think Laravel will last forever but we're definitely due a new paradigm switch in the PHP world, if you're not sure ask Drupal 6/7 developers about the dangers of framework dependent code.
Fluent interface is needlessly obscuring
Laravel's fluent interface coming out of Enquery::find()
makes it unnecessarily difficult to figure out what reject
and map
are operating on and its unclear where the result of map
are in that object, is there a way to extract them? If so I guess they are an array of nulls now? I can't tell from this call point.
The main problem with the fluent interface is that it's robbed you of the opportunity to name the result of your intermediate operation, a name which shows code intention is worth way more than your implementation code in code review and code maintenance.
Overly generic method name
The method name handle
will probably not stay adequate if you start tweeting anything else in your system or add any other methods to tweet
.
Handle what? Why? When? What operation is done when something is "handled"? Maybe we can call it tweet_enquiry
or a name that suggests that we tweet the enquiry based on NewEnquiryEvent
you will know this answer better than I do, translate what you know about the context into your code.
Below I've created some code that telegraphs to the reader certain information that I've made assumptions about all through reading the code
<?php
namespace App\Listeners\Enquiry;
use App\Events\NewEnquiryEvent;
use App\Exceptions\DebuggableException;
use Illuminate\Queue\InteractsWithQueue;
use Illuminate\Contracts\Queue\ShouldQueue;
use App\Models\Stores\Enquiry;
class Tweet
{
/**
* On enquiry event send a enquiry tweet if enquiry should do.
*
* @parm Enquiry
* The enquiry object.
*/
public function tweet_enquiry_on_event(NewEnquiryEvent $event) : void
{
$event_id = (int) $event->body->id;
$enqueries = (array) Enquiry::find($event_id);
$enqueries_that_should_tweet = array_filter($enqueries, 'enquiry_should_tweet');
array_walk($enqueries_that_should_tweet, 'enquiry_send_to_twitter');
}
}
// These functions are probably best off near the enquiry object.
/**
* Should an enquiry object be able to tweet?
*
* @param Enquiry
* The enquiry object.
*
* @return bool
* Whether an enquiry should be able to tweet.
*/
function enquiry_should_tweet(Enquiry $enquiry) : bool
{
$enquiry_should_tweet = $enquiry->shouldTweet();
return $enquiry_should_tweet;
}
/**
* Send the enquiry to twitter.
*
* @parm Enquiry
* The enquiry object.
*/
function enquiry_send_to_twitter(Enquiry $enquiry) : void
{
$enquiry->sendToTwitter();
}