1
\$\begingroup\$

I have this script that i use to get all clients with their events, main thing here is that i need to return all clients that have some events in specific periods and also i need to show clients that don't have events, so for example if i have group of 10 clients, and in that group 5 clients have some events, i need to show them with that events, AND i need to show other 5 clients without events, basically i need always to show ALL clients from specific group no matter if they have or don't have events and all that is paginated as you can see. This script works, but if someone could suggest me some improvements i would appreciate it.

$app->get('/api/calendar/group/{groupId}', function ($request, $response, $args) {
 require_once 'dbConnect.php';
 require_once 'shared/securityService.php';

 $queryParams = $request->getQueryParams();

  $total = getTotalNumberOfClientsInGroup($db, $args['groupId']);

  if(!$total) {
    return $response
    ->withStatus(202)
    ->withHeader('Content-Type', 'text/html')
    ->write('There are no attendants in selected group!');
  }

    $pageSize = $queryParams['pageSize'];
    $totalPages = ceil($total / $pageSize);
    $offset = ($queryParams['page'] - 1)  * $pageSize;

    $stmt = $mysqli->prepare("SELECT id, CONCAT_WS(' ', firstName, lastName) 
                AS clientName FROM client WHERE groupId = ? ORDER BY id LIMIT ? OFFSET ?");
    $stmt->bind_param("iii", $args['groupId'], $pageSize, $offset);
    $stmt->execute();
    $stmt->bind_result($id, $clientName);

    $clientsIds = array();

     while ($stmt->fetch()) {
      $clientsIds[] = $id; //get client ids so i can use it in next query where i will get all clients and their events
                            $allClients[] = array(
                                'clientId' => $id,
                                'clientName' => $clientName,
                                'carePeriods' => array()
                            ); //This is array of all clients in requested group, its used to extend array that i get in below query with clients that don't have any events, because i always need to show ALL clients from group
                        }

  $clientsIds = implode(',', $clientsIds);

   $clientStmt = $mysqli->prepare("SELECT cl.id as clientId, a.id as
             attendantId, ca.id as caringId, CONCAT_WS(' ', a.firstName, a.lastName) AS 
            attendantName, CONCAT_WS(' ', cl.firstName, cl.lastName) AS clientName,
             ca.startDate, ca.endDate FROM client cl LEFT JOIN caring ca ON ca.clientId =
             cl.id LEFT JOIN attendant a ON a.id = ca.attendantId WHERE ca.startDate >= ?
             AND ca.endDate <= ? AND cl.id IN ($clientsIds)");


        $clientStmt->bind_param("ss", $queryParams['startDate'], $queryParams['endDate']);
    $clientStmt->execute();
    $clientStmt->bind_result($clientId, $attendantId, $caringId, $attendantName, $clientName, $startDate, $endDate);

                        $data = array();

                        while ($clientStmt->fetch()) {
                            if (empty($data[$clientId])) {
                                $data[$clientId] = array(
                                    'clientId' => $clientId,
                                    'clientName' => $clientName,
                                    'carePeriods' => array()
                                );
                            }
                            if ($caringId) {
                                $found = false;
                                foreach ($data[$clientId]['carePeriods'] as $event) {
                                    if($event['caringId'] == $caringId){
                                        $found = true;
                                        break;
                                    }
                                }
                                if($found == false)
                                    $data[$clientId]['carePeriods'][] = array(
                                        'caringId' => $caringId,
                                        'startDate' => $startDate,
                                        'endDate' => $endDate,
                                        'attendantId' => $attendantId,
                                        'attendantName' => $attendantName
                                    );
                             }
                        }

                        foreach ($allClients as $value) {
                            $found = false;
                            foreach ($data as $key => $existingValue) {
                                if($existingValue['clientId'] === $value['clientId']) {
                                    $found = true;
                                    break;
                                }
                            }
                            if($found == false) {
                                $data[] = $value;
                            }
                        } //compare array that clientStmt returns with all clients array, and
                 // populate clientStmt array with clients that already dont exist there

                        $res['totalItems'] = $total;
                        $res['items'] = [];

                        foreach($data as $item) { 
                            $res['items'][] = $item; 
                        } //sort array with correct index, i use this because i want first element to have index 0, and not clientId

$clientStmt->close();
$stmt->close();
$mysqli->close();
echo json_encode($res);
 });
\$\endgroup\$
  • \$\begingroup\$ Please reformat your question making it fit without scrolling as it's nearly impossible to read your lengthy comments and queries \$\endgroup\$ – Your Common Sense Mar 24 '17 at 7:21
  • \$\begingroup\$ Please spend a little effort to make your code readable, in order to show that you do care for people whom you ask for help. Thank you. \$\endgroup\$ – Your Common Sense Mar 24 '17 at 7:41
  • \$\begingroup\$ Sorry i didnt notice this by now, i will reformat my code \$\endgroup\$ – Super Mario's Yoshi Mar 24 '17 at 7:43
  • \$\begingroup\$ Code is now reformated, sorry i wasn't able to do it better, but i think its now much better \$\endgroup\$ – Super Mario's Yoshi Mar 24 '17 at 9:46
  • 2
    \$\begingroup\$ @ShaunBebbers Do not edit your whitespace suggestions into the question (unless the author was obviously having difficulty conveying the code as intended). Complaints about whitespace are a matter addressable in an answer. I've rolled back Rev 3 → 2. \$\endgroup\$ – 200_success Aug 1 '17 at 17:02
4
\$\begingroup\$

I am guessing that there is some bootstrapping script which is instantiating your slim application an including this controller, so my first thought is, if that is the case, shouldn't fundamental includes like setting up database connections and including security functionality be part of that common bootstrapping process as opposed to being required here? Unless this is the only route (or one of few routes) on your application that leverage this functionality, I would find it odd to include here only in the context of this controller.


Should you validate presence and value of groupId parameter in request before trying to operate against it? I ask, because without knowing what happens in getTotalNumberOfClientsInGroup, you may have a case where you are doing things like making relatively expensive database queries before validating the input. You also might need to respond with 4XX error if the input is bad, which is something that is not clear from code shown that you are doing.


Again you start working with pageSize and page parameters before doing any validation against it. I am guessing this controller would benefit greatly from some upfront validation. You go so far as to prepare a query against the database before performing any validation when you should bail with 4XX response before getting to this point.


You should be able to retrieve all your data with a single query here. You should consider turning first query into a subselect within second query to either JOIN against or using within WHERE IN() clause. This is likely to simplify your code to map into your data structure as well.


I would suggest using exact comparison operators === and !== as default comparisons on your coding vs. loose comparisons. Using unnecessary loose comparisons tends to make you code more fragile to unexpected truthy/falsely behaviors. You should use loosed comparisons only when there is a specific reason to do so.


/**
 * sort array with correct index, i use this because 
 * i want first element to have index 0, and not clientId
 */
foreach($data as $item) { 
    $res['items'][] = $item;
} 

Build your data structure the way you want it to begin with so you can get rid of unnecessary code like this.


$clientStmt->close();
$stmt->close();

Why are you holding both of these statements open well after they are still needed in code?


All of your DB operations are happy-path. You just assume they will work (which is especially problematic based on data validation concern noted earlier). What is prepares fail? What if execution fails?


For your queries, don't just break them up arbitrarily across lines (which I assume you did based on comments to post), structure them in a meaningful/readable way. For example:

SELECT
    cl.id as clientId,
    a.id as attendantId,
    ca.id as caringId,
    CONCAT_WS(' ', a.firstName, a.lastName) AS attendantName,
    CONCAT_WS(' ', cl.firstName, cl.lastName) AS clientName, 
    ca.startDate,
    ca.endDate
FROM client cl
LEFT JOIN caring ca
    ON ca.clientId = cl.id
LEFT JOIN attendant a
    ON a.id = ca.attendantId
WHERE
    ca.startDate >= ? 
    AND ca.endDate <= ?
    AND cl.id IN ($clientsIds)

This makes complex queries much easier to read. Don't be afraid to write the query into a variable (rather use directly in prepare()) if this makes your code more readable.

And even be consistent in your formatting within query. Your field aliasing seems arbitrary. You sometimes use uppercase for AS and sometimes don't (I would actually recommend uppercase for everything in query except database objects, columns names, and aliases which you generally seem to be doing).


To your DB schema itself. You may want to consider not using camelCase for names of database objects. snake_case is more or less a de facto standard for MySQL which has some peculiarities around how it handles casing in database objects.


You can often use ORDER BY in your queries to help you simplify how you read data from multi-table queries into multi-dimensional data structures.


Update

Per your request, here is how you might use your first query as a sub-select to simplify the overall query and data retrieval process:

SELECT
    cl.id AS clientId,
    cl.clientName AS clientName,
    a.id AS attendantId,
    CONCAT_WS(' ', a.firstName, a.lastName) AS attendantName, 
    ca.id AS caringId,
    ca.startDate AS startDate,
    ca.endDate AS endDate
FROM (
    SELECT
        id,
        CONCAT_WS(' ', firstName, lastName) AS clientName
    FROM client
    WHERE groupId = ?
    ORDER BY id
    LIMIT ? OFFSET ?
) AS cl
LEFT JOIN (
    SELECT
        id,
        clientId,
        startDate,
        endDate
    FROM caring
    WHERE
        startDate >= ? 
        AND endDate <= ?
) AS ca
    ON ca.clientId = cl.id
LEFT JOIN attendant AS a
    ON a.id = ca.attendantId

A word of caution though - sometimes when getting into more complex queries with subselects, the can get into performance issues. I understand you current two query approach, which at least has the benefit of using two straightfoward queries. Before moving to something like I have suggested, make sure you look at query performance to make sure it is somewhat inline with the combined response times you have from your two query approach. If you are dealing with response times that are an order of magnitude higher, you probably should not take this approach.

\$\endgroup\$
  • \$\begingroup\$ Thank you very much on so detailed review, i already started making those changes.I have more questions related to your suggestions: 1. When i should close statements and db connection, 2.Can you please provide some code example how i can retrieve all data with just one query, because im not sure how to do it, i need both queries because this resource is paginated and i need 20 clients on each page, so that is reason why i have 2 queries. \$\endgroup\$ – Super Mario's Yoshi Mar 24 '17 at 18:28
  • 1
    \$\begingroup\$ @SuperMario'sYoshi Close statement objects after you are done reading out of them. I typically wouldn't worry about explicitly closing the DB connection, as in most applications you try to set up a global connection and any one piece of code might not be properly positioned to understand when to close the connection. This is a good example of that. This code doesn't instantiate$mysqli so it shouldn't be responsible for closing it. You in fact already have a case where you return early and wouldn't explicitly close anyway. \$\endgroup\$ – Mike Brant Mar 24 '17 at 19:01
  • \$\begingroup\$ @SuperMario'sYoshi Updated my answer to include example of single query using subselect. \$\endgroup\$ – Mike Brant Mar 24 '17 at 19:08
  • \$\begingroup\$ Thx a lot for explanations, and your both query examples works, BUT main reason why i have 2 queries is because i need to show all clients, no mater if they have or do not have events, and your query examples returns me just clients with events that pass start and end date where clause, so i kinda don't have use of it. \$\endgroup\$ – Super Mario's Yoshi Mar 24 '17 at 19:32
  • 1
    \$\begingroup\$ Let us continue this discussion in chat. \$\endgroup\$ – Mike Brant Mar 24 '17 at 21:30

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.