Refactor the method which has the sequence of the similarly looking blocks of code to (or towards) the design pattern(s)

softwareengineering.stackexchange https://softwareengineering.stackexchange.com/questions/404017

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;
}
有帮助吗?

解决方案

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.

其他提示

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;
许可以下: CC-BY-SA归因
scroll top