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

Question

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

SMS
PUSH
FAX
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;

getTemplateCode() will return the template code i.e booking-accepted
$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

Hey, welcome to Code Review! It is not quite clear from your question if your code currently works, i.e. reaches the desired output. — Graipher
– Graipher, Commented Feb 3, 2020 at 8:26
Code works, but it looked messy, I want to refactor it. there are so many IFs, wanted to know if there is any other way of doing it. @Graipher — Syed Ameer Hamza
– Syed Ameer Hamza, Commented Feb 3, 2020 at 8:27
The current question title, which states your concerns about the code, is too general to be useful here. The site standard is for the title to simply state the task accomplished by the code. Please see How to Ask for examples, and revise the title accordingly. — Jamal
– Jamal, Commented Feb 16, 2020 at 6:01

mdexp · Accepted Answer · 2020-02-16 01:49:37Z

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.

Stack Exchange Network

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

1 Answer 1

You must log in to answer this question.

Hot Network Questions

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

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions