17
\$\begingroup\$

I've decided to go with OOP style and prepared statements, and so far I like it a lot more than the procedural style. It's much more understandable in my opinion.

For this code review, I've just included my PHP code that interacts with my MySQL database. Everything is working how it should, but I'd like to know if you see anything that could be improved upon. Also, I'd like a security analysis/review of the code. Is my code safe from SQL injection since I'm using prepared statements? Is there anything else I should be doing?


I have this function in a folder/file above public_html, and just require it on the pages that need it.

function connectToDatabase() {
    $connect = new mysqli('localhost', 'myUserName123', 'myPassword123', 'myDatabaseName');
    if ($connect->connect_error) {
        exit;
    }
    return $connect;
}

This is the function I use to pull the question text, and answer integers from the database to display on the website.

function getQuestionsAndAnswers($connect) {
    $questionArray = array();
    $answersArray = array();
    $getStuff = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`<? ORDER BY `questionNumber` ASC');
    $getStuff->bind_param('i', $questnum);
    $questnum = 21;
    $getStuff->execute();
    $getStuff->bind_result($singleQuestion, $singleAnswer);
    while ($getStuff->fetch()) {
       array_push($questionArray, $singleQuestion);
       array_push($answersArray, $singleAnswer);
    }
    $getStuff->close();
    $connect->close();
    return array($questionArray, $answersArray);
}

This is the code that gets run after the user takes the math quiz, submits his answers, and I validate that the answers are all integers, etc.

$connect = connectToDatabase(); // open database connection here

$stmt1 = $connect->prepare("SELECT MAX(`id`) FROM QuizAdministration");
$stmt1->execute();
$stmt1->bind_result($maxId);
$stmt1->fetch();
$stmt1->close();

$stmt2 = $connect->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");
    for ($yty = 1; $yty < 21; $yty += 1) {
        $stmt2->bind_param('iii', $QuizAdministrationid, $questnum, $questansw);
        $QuizAdministrationid = $maxId + 1;
        $questnum = $yty;
        $questansw = $_POST['qora'.$yty];
        $stmt2->execute();
    }
    $stmt2->close();

$stmt3 = $connect->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");
$stmt3->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
$quizNumber = 1;
$playerCookie = session_id();
$playerIP = getUserIp();
$playersPercent = 90;
$stmt3->execute();
$stmt3->close();

$connect->close(); // close database connection here
header('Location: http://www.myWebsite.com/quizResults.php');
exit;
\$\endgroup\$
  • \$\begingroup\$ It looks like you are using prepared statements correctly. However, I'd suggest rewriting using PDO prepared statements with named parameters. All of those binding statements can go away. I can give you a short example if you want. \$\endgroup\$ – Cerad Mar 8 '14 at 16:21
  • \$\begingroup\$ Why do you separate questions and answers into two separate arrays? It seems to me that working with one array of question-answer pairs is much easier. But we'd have to see the code where you use those arrays to be sure. \$\endgroup\$ – Sebastian Redl Mar 12 '14 at 11:10
  • 1
    \$\begingroup\$ One thing, the first query you make should not be a prepared statement. The rule of thumb: If your query has parameters, it needs to be a prepared statement. Just selecting something from a table with no constraints or parameters can and should be a normal query (Prepared statements have a slight hit on performance). \$\endgroup\$ – Madara Uchiha Jun 11 '14 at 6:43
8
\$\begingroup\$

This looks pretty good. I'm glad to see the improvement.

You inexplicably call $connect->bind_params() using variables that have not yet been set.

The main issue I see now is $QuizAdministrationid = $maxId + 1, which is prone to a race condition. If two users submit the form simultaneously, it's possible that both pages could see the same result from SELECT MAX(id) … and generate the same next $QuizAdministrationid.

The solution is to ask the database to generate the next ID. Every SQL database supports this in a slightly different way. For MySQL, you should declare the id column of QuizAdministration to be AUTO_INCREMENT. Then, leave id column unspecified when you do the INSERT. MySQL will automatically fill in the id column with the next available number. To find out what number MySQL picked for you, use $connect->insert_id.

To make this work for you, you'll need to insert a row into QuizAdministration first, then use the id of the new row to insert the PlayerAnswers.

$stmt2 = $connect->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");
$quizNumber = 1;
$playerCookie = session_id();
$playerIP = getUserIp();
$playersPercent = 90;
$stmt2->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
$stmt2->execute();
$stmt2->close();

$quizAdministrationid = $connect->insert_id;

$stmt3 = $connect->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");
for ($yty = 1; $yty <= 20; $yty += 1) {
    $questnum = $yty;
    $questansw = $_POST['qora'.$yty];
    $stmt3->bind_param('iii', $QuizAdministrationid, $questnum, $questansw);
    $stmt3->execute();
}
$stmt3->close();
\$\endgroup\$
6
+50
\$\begingroup\$

The security aspect of things looks fine, but there are some areas for improvement, I have provided inline comments where possible.

The whole quiz could be coded as a class, and would be a lot nicer, however I didn't want to go too far from what you started with so you can see what changes I have done.

Overall, I have split the code into functions, and tried to make each function do 1 thing only, and renamed variables for readability. This will become more important when you go to maintain the code.

The first bit of code is what I used to test all the functions and catches exceptions that occur. You could improve by adding more error checks after each prepare and execute, but I will leave that to you, as they are only really necessary if the database structure doesn't match the sql.

try {
    $conn = connectToDatabase();

    list($questions, $answers) = getQuestionsAndAnswers($conn);

    // lets handle user data, separate of database calls
    $answers = extractPostAnswers($_POST);

    // not sure this is the best idea, if we have 2 simultaneous calls, both will get the same $maxId
    // i am unsure of your database design, but the way I am assuming it should work is
    // have an autoincrement column on the quiz administration table
    // insert into the quiz administration table and get the last_insert_id back
    // then use that id value to store in the answer table
    $maxId = selectMaxId($conn);

    $QuizAdministrationid = $maxId + 1;
    saveAnswers($conn, $QuizAdministrationid, $answers);

    $quizNumber = 1;
    saveQuizAdministration($conn, $quizNumber);

    $conn->close(); // close database connection here

    header('Location: http://www.myWebsite.com/quizResults.php');

} catch (Exception $ex) {

    // you should log the messages, and display a nice error for the user, this is just to give you idea of exception handling
    die($ex->getMessage());
}


exit;




function extractPostAnswers($post) {

    $answers = array();

    for ($questnum=1; $questnum<21; $questnum++) {

        // what if the $_POST doesn't exist? what is your default value
        //$questansw = $_POST['qora'.$yty];

        $key = "qora{$questnum}";
        $answers[$questnum] = isset($_POST[$key]) ? $_POST[$key] : NULL ; // assuming null is ok as a default?
    }

    return $answers;
}


function connectToDatabase() {
    $connect = new mysqli('localhost', 'root', '', 'test');

    if ($connect->connect_error) {
        // exit doesn't help anyone, at least let the user know there is a database error,
        // and perhaps log the error for your own use
        //        exit;
        throw new Exception('Database Connection Error: ' . $connect->connect_error);
    }

    return $connect;
}


function getQuestionsAndAnswers($connect) {
    $questionArray = array();
    $answersArray = array();

    // is getStuff a really good description of what the variable is?
    // $getStuff = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`<? ORDER BY `questionNumber` ASC');
    $stmt = $connect->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`<? ORDER BY `questionNumber` ASC');

    // what no error checking?
    if (!$stmt) {
        throw new Exception("Database Error: {$connect->error}");
    }

    // not quite sure why $questnum is a constant value, if so why bother having is as a bind param, it could go directly into the stmt
    $questnum = 21;

    $stmt->bind_param('i', $questnum);

    $stmt->execute();

    $stmt->bind_result($singleQuestion, $singleAnswer);


    while ($stmt->fetch()) {

        // array_push is great adding whole arrays, but a little overkill for single items, this is personal preference, not essential
        //array_push($questionArray, $singleQuestion);
        //array_push($answersArray, $singleAnswer);
        $questionArray[] = $singleQuestion;
        $answersArray[] = $singleAnswer;
    }
    $stmt->close();

    // are you sure you want to close the connection here, it is passed in as a param, what if it something else needs to use it after this function?
    // $connect->close();

    // i am not a big fan of returning multiple values like this, but I have no suggestion as I am not sure how you are using the values
    return array($questionArray, $answersArray);
}

function saveQuizAdministration($conn, $quizNumber) {
    $cookie = session_id();
    $ip = getUserIp();
    $score = 90;

    $stmt = $conn->prepare("INSERT INTO QuizAdministration (`quizNumber`, `cookie`, `ip`, `score`) VALUES (?, ?, ?, ?)");

    // lets make things easier to read, name the variables the same as the columns
    // $stmt->bind_param('issi', $quizNumber, $playerCookie, $playerIP, $playersPercent);
    $stmt->bind_param('issi', $quizNumber, $cookie, $ip, $score);

    $stmt->execute();
}


function saveAnswers($connection, $quizAdministrationId, $answers) {
    $stmt = $connection->prepare("INSERT INTO PlayerAnswers (`QuizAdministrationId`, `questionNumber`, `playerAnswer`) VALUES (?, ?, ?)");

    $questionNumber = NULL;
    $playerAnswer = NULL;

    // why not use the col names for variable names, then it would be a whole lot easier for me when I read the code for the first time
    $stmt->bind_param('iii', $quizAdministrationId, $questionNumber, $playerAnswer);

    // $yty is a cryptic variable name, what does it mean?
    // i have moved the loop into the function extractPostAnswers as it shouldn't be mixed with database code
    // for ($yty = 1; $yty < 21; $yty += 1) {
    foreach ($answers as $questionNumber => $playerAnswer) {

        // bind outside the for loop, that is whole point of binding to variables, the variables can change without needing to be re-binded
        //    $stmt->bind_param('iii', $QuizAdministrationid, $questnum, $questansw);

        // we are re-assigning the same value over and over, put it outside the loop, outside the function even
        // $QuizAdministrationid = $maxId + 1;

        // assignment not required, use $questnum as the loop variable
        // $questnum = $yty;

        // if $questansw is null do we even want to execute the stmt? why store a null value
        $stmt->execute();
    }

    $stmt->close();
}


function selectMaxId($connection) {

    // whats the point of using prepared statements for a static statement that is being called once???
    $stmt = $connection->prepare("SELECT MAX(`id`) FROM QuizAdministration");
    $stmt->execute();
    $stmt->bind_result($maxId);
    $stmt->fetch();
    $stmt->close();

    // if no row exists, $maxId = null, is that what you are expecting?

    return $maxId;
}
\$\endgroup\$
4
\$\begingroup\$

With PDO, you could have:

$getStuff = $pdo->prepare('SELECT `question`, `answer` FROM `Quizzes` WHERE `questionNumber`< :questionNumber ORDER BY `questionNumber` ASC');

$getStuff->execute(array('questionNumber' => 21));

$rows = $getStuff->fetchAll();

foreach($rows as $row)
{
   $questions[] = $row['question'];
   $answers[]   = $row['answer'];
}

It's up to you to decide if removing the binding stuff makes the code any easier to read and maintain.

\$\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.