Multiple choice game

Question

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

Answer 1

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.

Answer 2

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

• James111: How would I fix this? Would I use a prepared STMT? Like explained on here - php.net/manual/en/pdo.prepared-statements.php
• 200_success: Yes, that would be the best approach.