Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

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 '15 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 '15 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 '15 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 '15 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 '15 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.