2
\$\begingroup\$

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

\$\endgroup\$
3
  • \$\begingroup\$ Hey, welcome to Code Review! It is not quite clear from your question if your code currently works, i.e. reaches the desired output. \$\endgroup\$ – Graipher Feb 3 '20 at 8:26
  • 1
    \$\begingroup\$ 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 \$\endgroup\$ – Syed Ameer Hamza Feb 3 '20 at 8:27
  • \$\begingroup\$ 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. \$\endgroup\$ – Jamal Feb 16 '20 at 6:01
4
\$\begingroup\$

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.

\$\endgroup\$

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.