0
\$\begingroup\$

Background.

A User can submit an enquiry via a form hosted on a clients website, these are pushed onto a queue, and then taken off the queue for processing, to process several different parts of the enquiry asynchronously a single event is fired, this event is listened for by several different listeners, each one with a unique and distinct set of functionality.

This is the complete listener I've written to post the inbound Enquiry to twitter.

<?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
{
    public function handle(NewEnquiryEvent $event)
    {
        Enquiry::find($event->body->id)->reject(function ($enquiry) {
            return !$enquiry->shouldTweet();
        })->map(function ($enquiry) {
            return $enquiry->sendToTwitter();
        });
    }
}
\$\endgroup\$
1

1 Answer 1

4
\$\begingroup\$

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();
}
\$\endgroup\$
10
  • \$\begingroup\$ What's the best way to address some of the points, theres a couple I agree on, a couple that I think are unwarranted in the context, but may be wrong. Should I update the question or post multiple comments ? \$\endgroup\$
    – Luke
    Aug 23, 2017 at 12:37
  • 1
    \$\begingroup\$ To be honest I do not know, comments don't support syntax highlighting for code, so maybe a new post or a edit to your original post? \$\endgroup\$
    – arcanine
    Aug 23, 2017 at 13:34
  • \$\begingroup\$ Updated, but left original intact so anyone in future can see what changes were made and the justifications for / against making them. I'm not a huge CR user so happy to drop a bounty on this once eligible as your answer gave me some real food for thought. \$\endgroup\$
    – Luke
    Aug 23, 2017 at 20:31
  • 1
    \$\begingroup\$ I've updated the review, it goes off on a tangent but hopefully the journey is more helpful than the destination \$\endgroup\$
    – arcanine
    Aug 24, 2017 at 9:38
  • \$\begingroup\$ Awesome feedback, want to give more than an accept and an upvote but doesn't look like I can bounty it after the fact... \$\endgroup\$
    – Luke
    Aug 24, 2017 at 10:39

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.