2

I try to refactor my code, with this one I got a bad performance. The goal is to get a list of arrays with unique "answer_id" , and to get some "Score", "Votes" and so one.

In my existing one first I check if $most_voted is null, if it is, I assign first element, and after that I will be just in second foreach where or I insert a new element, or I update an existing one. But from my second foreach I get a bad time. Any suggestions about this logic?

$answers = $history->toArray(); //here I have an array of arrays
$most_voted = [];

foreach ($answers as $key => $answer) {

    if (!empty($most_voted) ) {

        // in this if I create new arrays or I update existing ones 
        foreach ($most_voted as $k => $v) {
            //If value already exists, increase Votes/Score/Percentage
            if (intval($most_voted[$k]['answer_id']) === intval($answer['lkp_answer_id'])) {

                $most_voted[$k]['Votes'] = $most_voted[$k]['Votes'] + 1;
                $most_voted[$k]['Score'] = $most_voted[$k]['Score'] + $answer['score'];
                $most_voted[$k]['Percentage'] = substr((($most_voted[$k]['Votes'] * 100) / $votes), 0, 5);
                $most_voted[$k]['Weight'] = substr((($most_voted[$k]['Score'] * 100) / $total_scoring), 0, 5);

                //Else add new array element
            } else {
                $name = LkpAnswer::where('id', '=', $answer['lkp_answer_id'])->pluck('name');
                (isset($name[0]) && $name[0] !== null) ? $name = $name[0] : '';                

                if(! empty($answer['lkp_answer_id'])){

                    $most_voted[$key] = [
                        'answer_id' => $answer['lkp_answer_id'],
                        'Name' => $name,
                        'Votes' => 1,
                        'Score' => $answer['score'],
                        'Percentage' => substr(((1 * 100) / $votes), 0, 5),
                        'Weight' => substr((($answer['score'] * 100) / $total_scoring), 0, 5),
                    ];
                }
            }
        }
    } else {
        $name = LkpAnswer::where('id', '=', $answer['lkp_answer_id'])->pluck('name');
        (isset($name[0]) && $name[0] !== '') ? $name = $name[0] : '';
        //If $most_voted is null, insert first value
        $most_voted[$key] = [
            'answer_id' => $answer['lkp_answer_id'],
            'Name' => $name,
            'Votes' => 1,
            'Score' => $answer['score'],
            'Percentage' => substr(((1 * 100) / $votes), 0, 5),
            'Weight' => substr((($answer['score'] * 100) / $total_scoring), 0, 5),
        ];
    }

}

Here I dd($answers);

  0 => array:14 [
    "id" => 68
    "user_id" => 57
    "round_number" => 1
    "lkp_answer_id" => 15
    "question_id" => 65
    "paragraph_id" => null
    "answer_time" => 386
    "answer_type" => "none"
    "answer_type_second_menu" => null
    "score" => 1
    "user_score_for_answer" => 2
    "correct_answer_score" => 2
    "created_at" => null
    "updated_at" => null
  ]
  1 => array:14 [
    "id" => 262
    "user_id" => 44
    "round_number" => 1
    "lkp_answer_id" => 26
    "question_id" => 65
    "paragraph_id" => null
    "answer_time" => 716
    "answer_type" => ""
    "answer_type_second_menu" => null
    "score" => 1
    "user_score_for_answer" => 2
    "correct_answer_score" => 1
    "created_at" => null
    "updated_at" => null
  ] //and many more..
Beusebiu
  • 1,433
  • 4
  • 23
  • 68
  • 1
    well, first you can use type casting rather than using `intval()`. intval is slower than direct typecasting`(int)` https://wiki.phpbb.com/Best_Practices:PHP – Anuj Shrestha Jun 04 '20 at 05:38
  • Cool, I will try this, but the speed is not changing that much. My bad performance is from the second foreach, but I am not sure what else to try.. – Beusebiu Jun 04 '20 at 05:41

1 Answers1

2

First rule of optimzation (in my view),

Never run queries inside a loop.


Before the loop starts:

// Fetch all data at once and use select() whenever possible
$LkpAnswers = LkpAnswer::whereIn('id', collect($answers)->pluck('lkp_answer_id')->toArray())->select('name')->get();


In your else condition (inside 2nd loop) do something like this.

// Before, this query was inside 2 for loops (n^2). For every n, there's 1 query.
// This right here is a Collection of LkpAnswer::model. Collection is very useful for optimization
$name = $LkpAnswers->where('id', $answer['lkp_answer_id'])->pluck('name')->toArray();

Use this and change in your second else condition as well. Just by doing this you will be cutting of almost 50% of your run time. Also check out this answer on how to optimize in laravel. It talks about caching, indexing, select() and type casting.

Keep me posted in the comments below.

Digvijay
  • 7,836
  • 3
  • 32
  • 53
  • 1
    I never expected for such a performance change. From 8/9 sec to 0.5, thanks a lot! – Beusebiu Jun 04 '20 at 05:55
  • "Never run queries inside a loop", it depends on what you are querying on. For RDBMs it may be true, for NoSQL(cassandra, redis etc) it is not. – Ersoy Jun 04 '20 at 09:03