0

I need some help to understand if the code below could be refactored to something less straightforward, less repetitive and more towards any appropriate pattern.

What I feel uncomfortable with in the code is the flow of repetitive tasks with the same pattern like:

// Get the result from some operation (API call / or any other operation);
// Check if the result is somehow valid;
// If it is not valid, set the response object accordingly and return early;
// If it is valid, continue with the next step with the overall same logic but different details.

Does the code look like being able to get refactored to (or towards) some usefully applicable here design pattern?

Would love to hear any feedback on it.

Here is the code:

/**
 * Check if the given email exists in the SendGrid recipients global list
 * and its custom field 'status' has the value 'subscribed'.
 *
 * @param  string  $email The email to check.
 *
 * @return object  (object)['isfound'=>false, 'issubscribed'=>false];
 */
public function getSubscriberStatus(string $email): object
{
    $result = (object) ['isfound' => null, 'issubscribed' => null];

    /**
     * Find the email in the SendGrid global list.
     */
    $endpoint = "contactdb/recipients/search?email=$email";
    $found = $this->callSendGrid('GET', $endpoint);
    if ($found->status !== 200) {
        Log::error(sprintf('[SENDGRID] Error while searching the email: %s in the SendGrid list, status: %s, message: %s', $email, $found->status, $found->message));
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    if (!($found->data->recipient_count > 0)) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    /**
     * Find the recipient with email exactly matching the required one.
     */
    $recipient = collect($found->data->recipients)->first(function ($item) use ($email) {
        return $item->email === $email;
    });

    /**
     * No exactly matching emails.
     */
    if (!$recipient) {
        $result->isfound = false;
        $result->issubscribed = false;
        return $result;
    }

    $result->isfound = true;

    /**
     * Get the status field of the recipient's 'custom_fields' array.
     */
    $status = collect($recipient->custom_fields)->first(function ($item) {
        return $item->name === 'status';
    });

    if ($status->value !== 'subscribed') {
        $result->issubscribed = false;
        return $result;
    }

    $result->issubscribed = true;
    return $result;
}
  • 1
    "early error return" is commonly solved by either using exceptions or by using Either/Error monads. Not sure which is more appropriate for PHP. – Euphoric Jan 20 at 6:44
  • @Euphoric, this is true and a completely valid point. Thank you for highlighting. The reason I did not use exceptions here is the fact that I early return with the status object, that is never an error. – bob-12345 Jan 20 at 7:41
  • 2
    refactored to something less straightforward why would you want to refactor a code to be "less straightforward"? Straightforward is good. – Lie Ryan Jan 20 at 10:06
  • @LieRyan, fully agree, good. Especially if the code is short. But in this case the code is rather long and to me is to much repetitve that (for me) it screams for refactoring, – bob-12345 Jan 20 at 11:26
1

This is a good use case for using Exception, if the language supports exception. You can just write:

public function getSubscriberStatus(string $email): object
{
    $endpoint = "contactdb/recipients/search?email=$email";
    $found = $this->urlget($endpoint, 200);

    // this validation is probably unnecessary, looping over empty object has the same result as not finding exact match
    // $this->validateHasRecipient($found); 

    $recipient = $this->findExactMatchRecipient($found, $email);
    $this->validaterRecipientIsSubscribed($recipient);

    return $recipient;
}

and let the validation methods raises exceptions if they fail. The exception object should contain detail about what's went wrong, so catcher can do if ($exc->isfound && !$exc->issubscribed) to handle recipients that are found but not subscribed specially.

If your caller actually requires a status object and you can't modify the caller to expect exception, you can just wrap the method in a try-block to convert that exception to a status object of the form that's expected by the caller.

| improve this answer | |
  • Ryan, thanks. This is generally correct move. However my design is throwing exception is not needed. If it is raised, I catch it and transform into the response that I return. See, whether this is the exception (API error) or just email not found, it still reflects into the same $response->isfound = false action. My pain in the entire method is the similar blocks repetition. – bob-12345 Jan 20 at 11:33
  • No, your exception handler simply just goes: $result = (object) ['isfound' => $exc->isfound, 'issubscribed' => $exc->issubscribed];. – Lie Ryan Jan 20 at 11:46
  • Ryan, ok. Got your point. As I understand the refactoring you propose it to hide all the logic on finding and veryfiing if the email is subscribed into the methods urlget, findExactMatchRecipient,validaterRecipientIsSubscribed (actually this is extract method refactoring). I suspect this with a couple of extract class refactorings (SubscriberStatusDTO and Subscriber, the latter will hold all your methods) will be my choice. Thank you for your kind help. – bob-12345 Jan 20 at 13:37
1

If instead of:

$result = (object) ['isfound' => null, 'issubscribed' => null];

the initial values were:

$result = (object) ['isfound' => false, 'issubscribed' => false];

most of the other code could be simplified. E.g.

if (!($found->data->recipient_count > 0)) {
    $result->isfound = false;
    $result->issubscribed = false;
    return $result;
}

would become:

if (!($found->data->recipient_count > 0))  return $result;
| improve this answer | |
  • Ray, thanks. This is as well a good advice. In the solution I plan to implement the $result object will be refactored into a class SubscriberStatusDTO. All the $result properties will be hidden there and set to false at the initialization. – bob-12345 Jan 21 at 4:47

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.