Refactoring of Eloquent Query - Properly The Worst Query I've Ever Written

To start with, I have this application developed using Laravel (It's an SMS application that anyone can signup for and send messages anywhere in the world (120+ countries literally).

Now for every country that I add from the admin section, I can manually assign a price for that country's network e.g. in the USA, we've various telcos like AT&T, Verizon, etc. now each of them may have a different cost for sending a single SMS and this is the same way it's implemented.

Assuming I want to send messages to 10k contacts (phone numbers) the current query I have would have to check through each country I have saved in my database, check the various countries network and get their respective prefix (the prefix is the country code, in the USA it's +1, Australia if +61 and it goes on like that).

After it does this check then it will get the respective prices set for those country's network and it will have to calculate by the number of contacts being sent to in this case 10k then it will add by the number of pages of SMS typed. (a 1 page SMS is made up of 160 characters).

All this calculation has to go on, then a modal pops up and informs the user of the total contacts they're sending to, the cost of sending it, and their current account balance.

This current query seems to work for small volumes maybe 500 (in terms of speed in processing and doing all these calculations before the modal pops up for the user to confirm before the messages are sent).

I need a way to refactor this and I'm open to all opinions and suggestions from the community.

Thanks in advance and below in the code in my helper file.

	public static function get_mobile_number_network(User $user, $mobile_number): array {
    $mobile_number = self::format_mobile_number($mobile_number);
    foreach (Network::with('country')->get() as $network) {
        $prefixes = explode(',', $network->prefix);
        foreach ($prefixes as $prefix) {
            $prefix = trim($prefix);
            if (starts_with($mobile_number, $prefix)) {
                $usr_price = UserNetwork::where([
                    ['user_id', '=', $user->id],
                    ['network_id', '=', $network->id],
                    ['country_id', '=', $network->country_id],
                ])->first();
                return [
                    'network_id' => $network->id,
                    'network' => $network->name,
                    'country_id' => $network->country_id,
                    'country' => $network->country->name,
                    'new_sms_price' => empty($usr_price) ? $network->new_sms_price : $usr_price->new_sms_price,
                    'hlr_price' => empty($usr_price) ? $network->hlr_price : $usr_price->hlr_price,
                ];
            }
        }
    }
    return [
        'network_id' => null,
        'network' => null,
        'country_id' => null,
        'country' => null,
        'new_sms_price' => $user->credit->price,
        'hlr_price' => $user->credit->hlr_price,
    ];
}

chaudigv

5 years ago

Level 16

There are few parameters you must take care of. Never have queries inside a loop.

/**
* 1. Select only the columns you need.
* 2. Since networks table data is not gonna change a lot, it's better to cache the results.
*/
$networks = Network::select('id', 'prefix', 'country_id', 'name', 'new_sms_price', 'hlr_price')->with('country:id,name')->get();

/**
* 1. Fetch relevant data all at once rather than quering inside loops.
*/
$userNetworks = UserNetwork::select('network_id', 'country_id', 'new_sms_price', 'hlr_price')
    ->where('user_id', $user->id)
    ->whereIn('network_id', $networks->pluck('id')->toArray())
    ->whereIn('country_id', $networks->pluck('country_id')->toArray())
    ->get();

foreach ($networks as $network) {
    if (starts_with($mobile_number, $prefix)) {
        // This right here, is the use of collection
        $usr_price = $userNetworks->where('network_id', $network->id)
            ->where('country_id', $network->country_id)
            ->first();
    }
}

The above code is a good start in slashing down multiple queries. Eager to see what others will recommend.

1 like

VimKanzo

5 years ago

Level 1

Hello @chaudigv so I tested it out and there seems to be much improvement in the time it takes to execute it but then I noticed something funny.

When I input a number to send an SMS to say, 233242536521 (this is a Ghana number based on the country code), the code above is supposed to check based on the prefix of that specific country network (which in this example is 23324), but it's rather running another country (Israel from the network query we have above which isn't right).

Below is the re-written code based on your suggestion. Thanks in advance for the assistance.

NB: I'm using a laravel package for the caching (cacheFor) on the model level for the Country, Network and UserNetwork Models.

public static function get_client_network_price($user, $mobile_number): array {
    $mobile_number = self::format_mobile_number($mobile_number);
    $network = Network::select('id', 'prefix', 'country_id', 'name', 'new_sms_price', 'hlr_price', 'voice_price')
            ->with('country:id,name')
            ->cacheFor(604800)
            ->get();

    $usr_price = UserNetwork::select('network_id', 'country_id', 'new_sms_price', 'hlr_price', 'voice_price')
                ->where('user_id', Auth::id())
                ->whereIn('network_id', $network->pluck('id')->toArray())
                ->whereIn('country_id', $network->pluck('country_id')->toArray())
                ->whereIn('country_id', $network->pluck('country_id')->toArray())
                ->cacheFor(604800)
                ->first();

    foreach ($network as $netwrk) {
        $prefixes = explode(',', $netwrk->prefix);
        foreach ($prefixes as $prefix) {
            $prefix = trim($prefix);
            if (starts_with($mobile_number, $prefix)) {
                $usr_price->where('network_id', $netwrk->id)
                        ->where('country_id', $netwrk->country_id)
                        ->first();
            }

            return [
                'network_id' => $netwrk->id,
                'network' => $netwrk->name,
                'country_id' => $netwrk->country_id,
                'country' => $netwrk->country->name,
                'new_sms_price' => empty($usr_price) ? $netwrk->new_sms_price : $usr_price->new_sms_price,
                'voice_price' => empty($usr_price) ? $netwrk->voice_price : $usr_price->voice_price,
                'hlr_price' => empty($usr_price) ? $netwrk->hlr_price : $usr_price->hlr_price,
    ];
        }
        return [
        'network_id' => null,
        'network' => null,
        'country_id' => null,
        'country' => null,
        'sms_price' => $user->credit->price,
        'hlr_price' => $user->credit->hlr_price,
        'voice_price' => $user->credit->voice_price,
    ];
    }
}

VimKanzo

5 years ago

Level 1

Thanks very much, @chaudigv for the time taken to read and to record the code above. I will test this out and see if we would see some great improvement as the main issue is it takes forever when the mobile numbers it's supposed to process are many.

Please or to participate in this conversation.