1
\$\begingroup\$

This is a multiple choice game. It has a web page where you enter a question, which can have multiple answers along with multiple options.

Can anyone recommend any improvements for my code?

//correct & options are both arrays. These arrays are being passed through an AJAX call
function addQuestion($topicID, $difficulty, $isMultiple, $quest,$correct, $options){
    global $numRecords,$dbConnection,$stmt;
    connect();

    //Insert into question table & recieve the question ID for use in the answers insert stmt
     $sql = "INSERT INTO question (topic_ID,difficulty,isMultiple, question)
            VALUES($topicID,$difficulty,$isMultiple,'$quest');";
    try{
        $stmt=$dbConnection->query($sql);
            $qID = $dbConnection->lastInsertId();

        if(!$stmt)
            die("error1".$dbConnection->errorInfo());
        $stmt = "";

        for($i = 0; $i < sizeof($correct); $i++):
            $stmt .= "INSERT INTO answer (question_ID, data, isCorrect)
                VALUES($qID,'".$correct[$i]."',1);";
        endfor;

        for($z = 0; $z < sizeof($options); $z++):
            $stmt .= "INSERT INTO answer (question_ID, data, isCorrect)
                VALUES($qID,'".$options[$z]."',0);";
        endfor;
        $dd = $dbConnection->query($stmt);

        if(!$dbConnection)
            die("error2".$dbConnection->errorInfo());               

    }catch (Exception $e){
        //Rollback if fails
        $dbConnection->rollback();
        return "Error rolling back: ". $e;
    }
}
\$\endgroup\$
1
\$\begingroup\$

Apart from the SQL injection, I think your code could do with some legibility improvements, as well as using some key features of php (like shorthand if/else and the foreach loop) to make your code simpler. I am going to assume that $dbConnection will contain a PDO connection that you can use for prepared and safe statements. I have added some comments, and I might be of on what types you expect your arguments to have, but in essence I think this reads much more clearly. You could (and probably should) also add a short line that describes what the parameter is for, so another developer in the future will understand at a glance.

// Add a question with answers to the database
function addQuestion(
      $topicID,    // Int
      $difficulty, // Int
      $isMultiple, // Bool
      $quest,      // String   
      $correct,    // [String]
      $options     // [String]
){
    // I will assume that $dbConnection is already a PDO object
    global $dbConnection;

    try {
        // Spaces and line breaks make your code easier to read.
        $query = $dbConnection -> prepare('
            INSERT INTO question  
            (topic_ID, difficulty, isMultiple, question)
            VALUES (?, ?, ?, ?)
        ');
        $query = $query -> execute($topicID, $difficulty, $isMultiple, $quest);

        // GET Id of Question just Inserted, or 0 if it failed
        $id    = $query ? $dbConnection -> lastInsertId() : 0;

        // Correction: I do understand why it used die. Its the try/catch thingy... Dumb of me!
        if(!$id) die('Error: ' . $dbConnection -> errorInfo());

        // Prepared statements also allow for 
        // repeated use with different values. This is our single setup.
        $query = $dbConnection->prepare('
            INSERT INTO answer
            (question_ID, data, isCorrect)
            VALUES (?, ?, ?)
        ');

        // Now we can execute all with different values, 
        // while reusing the prepared statement.
        // Also, a foreach loop is much more useful
        foreach($correct as $answer)
            $query -> execute($id, $answer, 1);
        foreach($options as $answer)
            $query -> execute($id, $answer, 0);

    } catch (Exception $e){
        // Rollback if fails
        $dbConnection -> rollback();
        return $dbConnection -> rollback()
            ? 'An error occured - rollback was successfull (' . $e . ')';
            : 'An error occured - rollback failed (' . $e . ')';
    }
    // It might be good to return something anyhow on success.
    // I use false so you can do if(!addQuestion(...)){ /* do something */ }
    // but it makes more sense to return true. But since a string evaluates
    // to true, it might be easier to use if it returns false
    return false;
}

Another note: your table names are not exactly feeling right with me. I would have gone for Questions and Answers - both capitalised to indicate that they are a table, and not a column. They are also in plural as they contain multiple questions and answers. Descriptiveness is good in this case, as once you have a lot of tables it is good to have them named very specifically.

\$\endgroup\$
3
\$\begingroup\$

Your code is vulnerable to . For example, if $quest contains a ' character, your INSERT will break.

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