Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

The purpose of this code is to get data from the database and output it into a multidimensional array in JSON for my web app. But my gut instinct is telling me this code is way too messy and just generally badly written.

// Structure of user_medication http://i.imgur.com/gTrZm7B.png

public function getUserMedication($data, $success, $fail){
        Permission::required(1, function() use($data, $success, $fail){
                $user = $data["user"];
                if(!$this->isUserUIDValid($user)){
                        $fail(["detail"=>"user does not exist!"]); return;
                }
                $medicationDates = $this->database->rowQuery("SELECT * FROM (SELECT *, id AS date_id FROM user_medication_dates WHERE user_uid = ? ORDER BY date DESC LIMIT 2) AS umd LEFT JOIN user_medication AS um ON umd.date = um.date ORDER BY umd.date DESC", array($data["user"]));
                $processedMedications = [];
                foreach ($medicationDates as $key => $value) {
                        if(!isset($processedMedications[$value["date_id"]])){
                                $processedMedications[$value["date_id"]]["date"] = $value["date"];
                        } elseif(!isset($processedMedications[$value["date_id"]][$value["time"]])){
                                $processedMedications[$value["date_id"]][$value["time"]] = [];
                        }
                        $processedMedications[$value["date_id"]][$value["time"]][] = $value;
                }
                $success($processedMedications);
        });
}

/* The output in JSON;

{  
   "status":"success",
   "data":{  
      "96":{  
         "date":"2015-05-30",
         "am":[  
            {  
               "id":"90",
               "date":"2015-05-30",
               "user_uid":"553669eea746d",
               "date_id":"96",
               "medication":"test",
               "dose":"1",
               "quantity":"2",
               "time":"AM"
            }
         ],
         "pm":[  
            {  
               "id":"91",
               "date":"2015-05-30",
               "user_uid":"553669eea746d",
               "date_id":"96",
               "medication":"another",
               "dose":"1",
               "quantity":"2",
               "time":"PM"
            }
         ]
      }
   }
}

*/
share|improve this question
2  
My gut agrees with your gut. Good question! Is there a reason you wrote a SELECT in a SELECT? –  Mast May 30 at 16:26
1  
Its a JOIN query to assure that only the last 2 dates from the dates table are selected. This way, we can select all the medications for the last 2 dates in the medication dates table. –  James Heald May 30 at 16:27
    
Not a big fan of having two statements in the same line: imagine you remove the {}, the indentation will not show you the error. –  Julien Palard May 31 at 15:44
    
Not a big fan too of the Permission::required very opaque role, neither on the way to return values only using function pointers... Why not just using return ? –  Julien Palard May 31 at 15:49
    
Not a big fan on writing code behind the 80th column, your SQL statement is hard to read on my phone, better correctly indent it. –  Julien Palard May 31 at 15:50

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.