0
\$\begingroup\$

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;

        // throw new \Exception("Error while searching the email: $email in the SendGrid list");
    }

    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;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to CodeReview@SE. While waiting for answers, have (another) look at How do I ask a Good Question? \$\endgroup\$ – greybeard Jan 19 at 14:16
  • \$\begingroup\$ (down-voters please comment.) \$\endgroup\$ – greybeard Jan 19 at 14:16
  • \$\begingroup\$ Please revisit the paragraph on titling your question. \$\endgroup\$ – greybeard Jan 19 at 14:44
  • 1
    \$\begingroup\$ Does the title state what your code does? Does it omit these kinds of superfluous phrases (Need an advice)? \$\endgroup\$ – greybeard Jan 19 at 16:29
  • 1
    \$\begingroup\$ The current question title, which states your concerns about the code, is too general to be useful here. Please edit to the site standard, which is for the title to simply state the task accomplished by the code. Please see How to get the best value out of Code Review: Asking Questions for guidance on writing good question titles. \$\endgroup\$ – Toby Speight Jan 20 at 8:29
3
\$\begingroup\$

I share your concern regarding the code presented to contain repetitions.

Design patterns are something to notice and use on an architectural level and do not apply here.

Suggestion:
• use in-line-comments for one-liners

(I kept several things the way you coded them (and, while not objecting strongly, I'd rather not): one guideline with say meaning code:
Say what you mean, mean what you say. (Scott D. Meyers(?)))
The @return doc comment will need improving, some lines remain overly long:

/**
 * 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' => false, 'issubscribed' => false];

    // 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));
        return $result;

        // throw new \Exception("Error while searching the email: $email in the SendGrid list");
    }

    if (!($found->data->recipient_count > 0)) {
        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) {
        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 = true;
    }

    return $result;
}
| improve this answer | |
\$\endgroup\$

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