3
\$\begingroup\$

Originally Posted on Stack Overflow

First of all I want to say that I'm new to PDO. I did tried it once but since I found the oop solution complicated and even impossible (SELECT * FROM table_name) I decided to use PDO.

But I'm not sure if I do it right, so I'd like to take criticism about what I've done. I'm "translating" the mysqli stmt to pdo. Here's an example from one thing I've "translated":

// Client IP has been defined previously,
// But for the example:
$ip     = '3ffe:1900:4545:3:200:f8ff:fe21:67cf';
$sql_ip = inet_pton($ip); // IPV6

try {
    $stmt = $pdo->prepare('SELECT * FROM sessions WHERE s_ipv4 = :s_ipv4 OR s_ipv6 = :s_ipv6');
    $stmt->bindParam(':s_ipv4', $sql_ip);
    $stmt->bindParam(':s_ipv6', $sql_ip);
    $stmt->execute();

    if ($stmt->rowCount() === 0) {
        // No rows
    } else {
        // Do something
    }
} catch (Exception $exception) {
    die ($exception->getMessage());
}

unset($stmt);

Am I doing it right?

\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review, and thank you for declaring your cross post. Please fill in the "do something" placeholders so that we can review this code properly. See How to Ask. \$\endgroup\$ – 200_success Dec 7 '17 at 14:18
2
\$\begingroup\$

There are two main points of improvement:

  1. First of all, never ever use die(error message) in your scripts. Neither try/catch should be ever used only to echo the error message out. You'd be surprised, but you will have more informative error message if just get rid of the whole try/catch/die stuff. Please read my article on PHP error reporting for the details

  2. PDO code itself could be made more tidy, thanks to various helper functions PDO offers

So the whole code should be

$sql_ip = inet_pton($ip); // IPV6

$stmt = $pdo->prepare('SELECT 1 FROM sessions WHERE s_ipv4 = ? OR s_ipv6 = ?');
$stmt->execute([$sql_ip,$sql_ip]);

if ($stmt->fetchColumn()) {
    // Do something
} else {
    // No rows
}

Here I used positional placeholders for brevity, and sent your variables directly to execute for the same purpose.

Also I recommend to avoid rowCount() function as it would be more logical: in your code you are selecting some data, but never use it. Instead, I would suggest to use the actual data selected (just a literal "1" in your case, fetched directly using fetchColumn() method).

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy