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.");
});
});
1 Answer 1
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 appropriatephp_value
s in your.htaccess
. Alternatively, and again on Apache, useSetEnv
in yourhttpd.conf
.$utf8
is a misleading name. It actually stores your PDO options. If you extend your options to includePDO::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
anduser_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}
.
-
\$\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– morgi2014年08月17日 13:26:31 +00:00Commented 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 atry
-catch
block and use a.rollBack()
in thecatch
. \$\endgroup\$Schism– Schism2014年08月17日 19:01:17 +00:00Commented 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– morgi2014年08月17日 22:50:21 +00:00Commented Aug 17, 2014 at 22:50