HiveBrain v1.2.0
Get Started
← Back to all entries
patternjavascriptMinor

Random select in two MySQL tables

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
randomtablestwomysqlselect

Problem

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.");
    });
});

Solution

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 }.

Context

StackExchange Code Review Q#60235, answer score: 5

Revisions (0)

No revisions yet.