4
\$\begingroup\$

I wrote a simple AJAX request that performs a random select in two MySQL tables. My current script involves connecting to the database and creating a new PDO every time the #generate button is clicked.

Can it be considered a bad practice? Could it lead to performance issues? If not, how could it still be optimized?

PHP (generator.php)

$host = "localhost";
$dbname = "generator";
$user = "root";
$password = "";
$utf8 = array(PDO::MYSQL_ATTR_INIT_COMMAND => "SET NAMES utf8");

try {
    $database = new PDO("mysql:host=$host; dbname=$dbname", $user, $password, $utf8);
    $database->setAttribute( PDO::ATTR_ERRMODE, PDO::ERRMODE_EXCEPTION );
}
catch(PDOException $e) {
    file_put_contents('PDOErrors.txt', $e->getMessage(), FILE_APPEND);
    die();
}

$query = "SELECT * FROM (
    (SELECT * FROM homemade WHERE validation = 1 ORDER BY RAND() LIMIT 0,1)
    UNION ALL
    (SELECT * FROM user_generated WHERE validation = 1 ORDER BY RAND() LIMIT 0,1)
)
AS foo
ORDER BY RAND()
LIMIT 0,1";

$statement = $database->prepare($query);
$statement->execute();
$content = $statement->fetch();

$data = array();

if($query) {
    $data["pickedName"] = $content["name"];
    $data["pickedBaseline"] = $content["baseline"];
}

echo json_encode($data);

JS

var $name = $("#content h2");
var $baseline = $("#content h3")

$("#generate").click(function() {
    $.ajax({
        type: "POST",
        url: "generator.php",
        dataType: "json"
    }).done(function(data) {
        $name.text(data.pickedName);
        $baseline.text(data.pickedBaseline);
    }).fail(function(data) {
        $name.text("Something went wrong !");
        $baseline.text("Please try again.");
    });
});
\$\endgroup\$

1 Answer 1

5
\$\begingroup\$

If you are careful about it, instead of creating a new PDO instance on every invocation of your script, you can use a persistent connection. Make sure you resolve all transactions through a commit() or rollBack().

To make your connection persistent, just include PDO::ATTR_PERSISTENT => true in your options array when constructing your PDO object.


And now, for a review of your code:

  • Storing database keys directly in your PHP file is considered bad practice. Generally it's better to put it somewhere outside your web root and require it in, or on Apache, to set appropriate php_values in your .htaccess. Alternatively, and again on Apache, use SetEnv in your httpd.conf.

  • $utf8 is a misleading name. It actually stores your PDO options. If you extend your options to include PDO::ATTR_PERSISTENT, then the variable name will no longer make sense.

  • Your query is inefficient. ORDER BY RAND() is slow enough as it is, and you're using it twice! (The third time is over two rows, so that's not a big issue.) Here's an excellent article on quickly selecting random rows in MySQL.

  • You're doing a SELECT *. It's better to explicitly mention what columns you're using.

  • If my assumptions are correct, homemade and user_generated have the same columns. I'm not sure this is a good idea; I think it would be better to put them in the same column. Note: as it is, your code will have 50% of picking a user generated pair of values, and 50% of picking a "homemade" pair, regardless of the ratio of user generated to homemade. You'll need to account for that if that's the desired behaviour.

  • Minor stylistic points: be consistent in spacing around parentheses. Also, catch is typically placed on the same line as the precending }.

\$\endgroup\$
3
  • \$\begingroup\$ Well, thanks for all this! I'll take a closer look at each point and make sure it's clean. Could you give me an example of a proper use of persistent connection in my case? It seems that it can turn out to be tricky if some transactions aren't committed. In terms of behaviour, the desired query should select randomly (and with a 50/50 ratio) a row from the two tables. \$\endgroup\$
    – morgi
    Aug 17, 2014 at 13:26
  • \$\begingroup\$ @morgi In that case (with the 50/50 thing), I suggest randomly choosing the table beforehand, in your PHP, before querying to the table. If you like, I can add an example to my answer. As for ATTR_PERSISTENT, since you aren't using transactions in your code, you should be able to put it in straight away. If you do switch to transactions at some point though (which, for this application, seems entirely unnecessary), just enclose it in a try-catch block and use a .rollBack() in the catch. \$\endgroup\$
    – Schism
    Aug 17, 2014 at 19:01
  • \$\begingroup\$ Choosing the table with PHP sounds like a pretty good idea, and I think I know how you meant it so I'll try. As for the persistent connection, I guess I just have to put it there and it will take care of itself? I thought it would somehow change the way I do the query. Thanks for your time! \$\endgroup\$
    – morgi
    Aug 17, 2014 at 22:50

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.