0
\$\begingroup\$

I'm using slim php and i have this simple paginated end point to get all clients, i feel like i could do some things here better, so i would appreciate all suggestions, if possible please give me code example with suggested improvements. Thank you in advance.

$app->get('/api/clients', function ($request, $response, $args) {
    require_once 'dbConnect.php';
    require_once 'shared/securityService.php';
    $queryParams = $request->getQueryParams();

    $total = getTotalCount($mysqli, $queryParams["group"]);
    $pageSize = $queryParams["pageSize"];

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

    if(is_numeric($queryParams["group"])) {
        $stmt = $mysqli->prepare("SELECT id, firstName, lastName, birthDate, phoneNumber, address,
            contactPerson, contactPersonPhoneNumber FROM client WHERE groupId = ? ORDER BY
            id LIMIT $pageSize OFFSET $offset");

        $stmt->bind_param("i", $queryParams["group"]);
    } else {
        $stmt = $mysqli->prepare("SELECT id, firstName, lastName, birthDate, phoneNumber, address,
            contactPerson, contactPersonPhoneNumber FROM client 
            ORDER BY id LIMIT $pageSize OFFSET $offset");            
    }

    $stmt->execute();
    $stmt->bind_result($id, $firstName, $lastName, $birthDate, $phoneNumber, 
        $address, $contactPerson, $contactPersonPhoneNumber);

    $data = null;

    while ($stmt->fetch()) {
        $data['items'][] = array(
            'id' => $id,
            'firstName' => $firstName,
            'lastName' => $lastName,
            'birthDate' => $birthDate,
            'phoneNumber' => $phoneNumber,
            'address' => $address,
            'contactPerson' => $contactPerson,
            'contactPersonPhoneNumber' => $contactPersonPhoneNumber
        );
    }

    $data['totalItems'] = $total;

    $stmt->close();
    $mysqli->close();
    echo json_encode($data);
});

function getTotalCount($mysqli, $groupId) {
    if(is_numeric($groupId)) {
        $totalStmt = $mysqli->prepare("SELECT COUNT(id) FROM client WHERE groupId = ?");
        $totalStmt->bind_param("i", $groupId);
    } else {
        $totalStmt = $mysqli->prepare("SELECT COUNT(id) FROM client");
    }

    $totalStmt->execute();
    $totalStmt->bind_result($total);
    $totalStmt->fetch();
    $totalStmt->close();
    return $total;
}
\$\endgroup\$
2
\$\begingroup\$

SQL Injection

You are vulnerable to SQL injection via pageSize.

You need to use prepared statements for all values, not just some of them.

Naming

total and getTotalCount could be more descriptive. Currently, I don't really know what "total" means without looking at the code of the function. You also don't need "total" at all (it's assumed when talking about a count). $clientCount = getClientCountForGroup($mysqli, $id) would be easier to understand.

Duplication

Your two queries are basically duplicates apart from the WHERE clause. You may want to remove that duplication.

\$\endgroup\$
  • \$\begingroup\$ Thx for suggestions, what is best way to remove duplication ? \$\endgroup\$ – Super Mario's Yoshi Mar 15 '17 at 19:27
1
\$\begingroup\$

PDO

Like I said before, moving to PDO will reduce the following code block

$stmt->bind_result($id, $firstName, $lastName, $birthDate, $phoneNumber, 
    $address, $contactPerson, $contactPersonPhoneNumber);

$data = null;

while ($stmt->fetch()) {
    $data['items'][] = array(
        'id' => $id,
        'firstName' => $firstName,
        'lastName' => $lastName,
        'birthDate' => $birthDate,
        'phoneNumber' => $phoneNumber,
        'address' => $address,
        'contactPerson' => $contactPerson,
        'contactPersonPhoneNumber' => $contactPersonPhoneNumber
    );
}

to

$data['items'] = $stmt->fetchAll();

Programming

It's always a good idea to create a function for the repetitive task. For running a query for example - an action that takes preparation, binding, execution, results retrieval. A function that takes a query and parameters as its arguments and returns the resulting data is always useful. A class would be better than a function though.

For example, it will make something like

function getClientCount($db, $groupId = 0) {
    if($groupId) {
        $stmt= $db->run("SELECT COUNT(id) FROM client WHERE groupId = ?",[$groupId]);
    } else {
        $stmt = $db->run("SELECT COUNT(id) FROM client");
    }
    return $stmt->fetchColumn();
}
\$\endgroup\$

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.