Refactor PHP | Laravel Code, Optimisation needed to make code clean

Question

I have made some notification preferences in which there are 4 major preferences

1. SMS
2. PUSH
3. FAX
4. EMAIL

And then there are some specific templates such as booking-accepted and within booking accepted there are again these 4 notifications.

So what I have to do I need to check 1st priority notification preferences first that if SMS is on then I have to check regarding specific templates that SMS is on in booking-accepted and then I'll send the notification or will just unset the recipient.

The object on notification preferences is as follows

{
    "sendSms": "1",
    "sendFax": "1",
    "sendEmail": "1",
    "sendPush": "0",
    "specific-templates": [
        {
            "update-video-physical-address": [
                {
                    "sendPush": "1",
                    "sendSms": "0",
                    "sendEmail": "0"
                }
            ],
            "booking-denied": [
                {
                    "sendPush": "1",
                    "sendSms": "0"
                }
            ]
        }
    ],
}

And The code I did for this is given below which is bit messy and unclean.

 foreach ($recipients as $recipient) {
            $user = $userRepository->findByParams([
                'email' => $recipient->getUser()->email
            ]);
            if (!is_null($user->userNotificationPreference) && array_key_exists($sendMethod, $user->userNotificationPreference->preferences)
                && $user->userNotificationPreference->preferences[$sendMethod] == true) {
                if (array_key_exists($notification->getTemplateCode(), $user->userNotificationPreference->preferences['specific-templates'][0])) {
                    if (array_key_exists($notification->getTemplateCode(), $user->userNotificationPreference->preferences['specific-templates'][0])
                        && array_key_exists($sendMethod, $user->userNotificationPreference->preferences['specific-templates'][0][$notification->getTemplateCode()][0])) {
                        if ($user->userNotificationPreference->preferences['specific-templates'][0][$notification->getTemplateCode()][0][$sendMethod] == false) {
                            $key = array_search($recipient, $recipients->toArray());
                            unset($recipients[$key]);
                        }
                    }
                }
            } elseif (!is_null($user->userNotificationPreference) && array_key_exists($sendMethod, $user->userNotificationPreference->preferences)
                && $user->userNotificationPreference->preferences[$sendMethod] == false) {


                $key = array_search($recipient, $recipients->toArray());
                unset($recipients[$key]);

            }
        }

        return $recipients;

1. getTemplateCode() will return the template code i.e booking-accepted
2. $sendMethod is method i.e sendSms, sendPush

Please help me with writing this mode more clean and efficient way such as reducing IFs and any other approach

Answer 1

You could exploit Laravel collections to achieve what you need.

Most of your code is checking for the presence/absence of props on your arrays. Laravel internally exposes and uses the Illuminate\Support\Arr class that contains a lot of useful methods to work with arrays without the hassles of checking for key existance.
It also allows you to use dot notation to get nested properties in a really simple way.

Laravel collection methods (documentation) use this class where needed. Therefore, the refactored code would become really intuitive and should look like this (it may need a little bit of tweaking based on your userNotificationPreference and preferences arrays and how you retrieve them from the model:

$recipients->map->getUser()
    ->where("userNotificationPreference.preferences.{$sendMethod}", true)
    ->where("userNotificationPreference.preferences.specific-templates.0.{$notification->getTemplateCode()}.0.{$sendMethod}", true);

Note that I used ->map->getUser() that would be equivalent to:

$recipients->map(function ($recipient) {
    return $recipient->getUser();
})

However, if you really need to pluck the user instance through the user repository then you can do:

$recipients->map(function ($recipient) use ($userRepository) {
    return $userRepository->findByParams([
        'email' => $recipient->getUser()->email
    ]);
})->where(...);

and chain the where calls after that.

If any part of my code is unclear or doesn't work, let me know in the comments and I'll explain/fix it.