Math quiz in PHP

Question

I coded a math quiz in PHP in which a user selects radio buttons as their answer choices. I retrieve the data of these ten radio buttons using $_POST[]; However, I have ten separate post queries for EACH radio button as well as SQL queries, and also ten separate if statements to check for each correct answer. It looks very clunky and bad coding practice. Does anyone have any tips to clean this code up? I figure using a loop or an array would maybe be a good start but it seems overwhelming.

//these variables hold which radio button the user selected
$answerChoice1 = $_POST['test1']; //pulls value of radio button named test 1
$answerChoice2 = $_POST['test2'];
$answerChoice3 = $_POST['test3'];
$answerChoice4 = $_POST['test4'];
$answerChoice5 = $_POST['test5'];
$answerChoice6 = $_POST['test6'];
$answerChoice7 = $_POST['test7'];
$answerChoice8 = $_POST['test8'];
$answerChoice9 = $_POST['test9'];
$answerChoice10 = $_POST['test10'];

$questionID1 = $_POST['theId1']; //pulls the 'bid' of the question asked
$questionID2 = $_POST['theId2'];
$questionID3 = $_POST['theId3'];
$questionID4 = $_POST['theId4'];
$questionID5 = $_POST['theId5'];
$questionID6 = $_POST['theId6'];
$questionID7 = $_POST['theId7'];
$questionID8 = $_POST['theId8'];
$questionID9 = $_POST['theId9'];
$questionID10 = $_POST['theId10'];

$sqlAnswer1 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID1 . "\"  "; //sql statement for selecting the questions that were generated
$sqlAnswer2 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID2 . "\"  "; //on the page
$sqlAnswer3 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID3 . "\"  ";
$sqlAnswer4 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID4 . "\"  ";
$sqlAnswer5 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID5 . "\"  ";
$sqlAnswer6 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID6 . "\"  ";
$sqlAnswer7 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID7 . "\"  ";
$sqlAnswer8 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID8 . "\"  ";
$sqlAnswer9 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID9 . "\"  ";
$sqlAnswer10 = "SELECT * FROM `math` WHERE `bid` = \"" . $questionID10 . "\"  ";

$sqlAnswerQuery1 = mysqli_query($link,$sqlAnswer1); //query for the select statements
$sqlAnswerQuery2 = mysqli_query($link,$sqlAnswer2);
$sqlAnswerQuery3 = mysqli_query($link,$sqlAnswer3);
$sqlAnswerQuery4 = mysqli_query($link,$sqlAnswer4);
$sqlAnswerQuery5 = mysqli_query($link,$sqlAnswer5);
$sqlAnswerQuery6 = mysqli_query($link,$sqlAnswer6);
$sqlAnswerQuery7 = mysqli_query($link,$sqlAnswer7);
$sqlAnswerQuery8 = mysqli_query($link,$sqlAnswer8);
$sqlAnswerQuery9 = mysqli_query($link,$sqlAnswer9);
$sqlAnswerQuery10 = mysqli_query($link,$sqlAnswer10);

$rowAnswer1=mysqli_fetch_array($sqlAnswerQuery1); //fetches the row of the questions asked on the page
$rowAnswer2=mysqli_fetch_array($sqlAnswerQuery2);
$rowAnswer3=mysqli_fetch_array($sqlAnswerQuery3);
$rowAnswer4=mysqli_fetch_array($sqlAnswerQuery4);
$rowAnswer5=mysqli_fetch_array($sqlAnswerQuery5);
$rowAnswer6=mysqli_fetch_array($sqlAnswerQuery6);
$rowAnswer7=mysqli_fetch_array($sqlAnswerQuery7);
$rowAnswer8=mysqli_fetch_array($sqlAnswerQuery8);
$rowAnswer9=mysqli_fetch_array($sqlAnswerQuery9);
$rowAnswer10=mysqli_fetch_array($sqlAnswerQuery10);

if ($answerChoice1 == $rowAnswer1['aanswer']){ //compares value of the 
                                           //radio button selected with
    $score = $score + 1;                       //the field of the correct answer
}                                              //if the two strings match, increment score by 1
else{
    $correctAnswer1 = false;
}


if ($answerChoice2 == $rowAnswer2['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer2 = false;
}

if ($answerChoice3 == $rowAnswer3['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer3 = false;
}

if ($answerChoice4 == $rowAnswer4['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer4 = false;
}

if ($answerChoice5 == $rowAnswer5['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer5 = false;
}

if ($answerChoice6 == $rowAnswer6['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer6 = false;
}

if ($answerChoice7 == $rowAnswer7['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer7 = false;
}

if ($answerChoice8 == $rowAnswer8['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer8 = false;
}

if ($answerChoice9 == $rowAnswer9['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer9 = false;
} 

if ($answerChoice10 == $rowAnswer10['aanswer']){
    $score = $score + 1;
}
else{
    $correctAnswer10 = false;
} 

Answer 1

You can simplify this dramatically, and protect against sql injection, by using the database to calculate the score, and using parameterized queries.

1st loop from 1 to 10 (inclusive), storing the question and corresponding answer sequentially into an array, here called params (eg parameters, for the sql query). In the same loop, you can begin to build the sql, using questionmark placeholders instead of the actual values

$params=[];
$sql = [];

for($i=1;$i<=10;$i++){
    $params[] = $_POST['theId'.$i];
    $params[] = $_POST['test'.$i];
    $sql[]= '( bid = ? AND aanswer = ? )';
}

Next we create the rest of the sql string by imploding are previous array of values:

$sqlString = 'SELECT COUNT(bid) FROM math WHERE ' . implode(' OR ', $sql);

The resulting string will look like 'SELECT COUNT(bid) FROM math WHERE (bid=? AND aanswer=?) OR (bid=? AND aanswer=?) OR ...'

The we create a PDO connection and use prepare and execute to bind our actual query parameters to the query:

$db = new PDO("mysql:dbname=test;host=localhost",'root','');    
$query = $db->prepare($sqlString);    
$query->execute($params);    
$score = $query->fetchColumn();

$score variable now holds the users score.

All together the code looks like:

$params=[];
$sql = [];

for($i=1;$i<=10;$i++){
    $params[] = $_POST['theId'.$i];
    $params[] = $_POST['test'.$i];
    $sql[]= '( bid = ? AND aanswer = ? )';
}

$sqlString = 'SELECT COUNT(bid) FROM math WHERE ' . implode(' OR ', $sql);


$db = new PDO("mysql:dbname=test;host=localhost",'root','');
$query = $db->prepare($sqlString);
$query->execute($params);
$score = $query->fetchColumn();

Answer 2

That may sound a bit harsh, but have you ever heard of the for loop ?

More on the SQL side of your code :

• SELECT * is BAD ! And that's really obvious in your code : the only field that you use is aanswer but you fetch all your fields. You may say that you only have two fields so it's not that important. Well in that case, this might be true. But what in 1 year ? Maybe your table will have 10 fields, that will all be selected if you don't change your query. Write it right once, you'll never have to change it again to do the same thing. And your server will thank you.
• You can fetch all the answers of all the questions answered in only one query. That will require a bit more of processing in PHP, but nothing you can't do. Hint : you may want to fetch the ID field of your table to be able to process it (the bid field if I understood well).
• mysqli_fetch_array is also pretty bad practice. Why ? Because it creates two arrays, one with numeric indexes, and one with associative indexes. Do you really need both and not just one ? I have to say, I've never seen an application that really needed both. mysqli_fetch_assoc is better.
• Do you have indexes on your mysql table, and if so, on what fields ? Right now that may not seem necessary, but if the number of questions/answers increase, you'll need at least one index on bid. The value of indexes is often under estimated, but as the number of entries in your table increase, that could be the difference between a query taking 10s and a query taking a few milli-seconds. The only cost is disk storage, but we don't live in the 90's anymore, so that should not be a problem.
• And you may to protect yourself from SQL injections (I'll let you find the appropriate fuctions to do that). Never Trust User Input !

Answer 3

Update

The original code I posted has an security vulnerability through a SQL Injection. I have attempted to correct this by using prepared statements, but I am not used to mysqli so there may be errors.

When the SQL query was built I injected the $questionIDs directly into the SQL query. This can lead to an SQL injection attack if a malicious user successfully alters the data contained inside one of the id's to contain malicious SQL commands. To remedy this I use prepared statements. They are more complex, but are very effective against SQL injection attacks. You should replace the query code with this below.

$statement = mysqli_prepare($link, 'SELECT `aanswer` FROM `math` WHERE `bid` IN(?) LIMIT ?');

mysqli_stmt_bind_param($statement, 'si', $questionsIDs, $numberOfQuestions);

/*
 * Execute the query and bind the 'answer' column to the variable
 * $column. If more columns are retrieved you would need to add more
 * variables to bind to.
 */
mysqli_stmt_execute($statement);
    mysqli_stmt_bind_result($statement, $column);

while(mysqli_stmt_fetch($statement)) {
    $correct[] = $column;
}

mysqli_stmt_close($statement);

Please make sure you have the same charset on both the connection and the database. Otherwise it can lead to injection attacks even with prepared statements.

Original

It is good you submit your code to review. Whats even better is that you know that it may have problems, which I think is a very good start. As you know: "The first step in solving a problem is recognizing there is one" :D

As you suggested you can reduce the overall size and readability with some loops and arrays. I have written an implementation that should do the exact same thing, but please keep in mind this have not been tested as I had no access to a database or a real IDE at the time of writing.

Along with the comments I think the code should be explanatory, but feel free to ask any questions about it. So without further ado, here we go!

/*
 * By storing the total amount of questions
 * in variable you can easily add new questions by increasing
 * this variable by the number of added questions. The only requirement
 * is that the new questions must follow the same naming pattern.
 */
$numberOfQuestions = 10;
$questionsIDs = [];

$answers = [];
$correct = [];
$score   = 0;

/*
 * Gather answer and question data. Since there should be an equal
 * amount of both we can combine this into one loop.
 */
for($iteration = 0; $iteration <= $numberOfQuestions; $iteration++) {

    /*
     * Create the appropriate POST keys and extract
     * the value while sanitizing a little.
     *
     * Sanitizing against a string can corrupt your data. Make sure all your
     * answers and questions only contain string characters. Otherwise you should change
     * the filters used in filter_input()
     */
    $key = 'test' . $iteration;     // Could use a more descriptive name
    $answers[] = filter_input(INPUT_POST, $key, FILTER_SANITIZE_STRING);

    $key = 'theId' . $iteration;    // Could use a more descriptive name
    $questionsIDs[] = filter_input(INPUT_POST, $key, FILTER_SANITIZE_STRING);

}

/*
 * Build SQL query.
 *
 * I used the IN clause to avoid another loop and appended
 * the LIMIT clause to help the SQL compiler optimize the query.
 *
 * It seems the column name 'aanswer' has a typo.
 */
$statement = mysqli_prepare($link, 'SELECT `aanswer` FROM `math` WHERE `bid` IN(?) LIMIT ?');

mysqli_stmt_bind_param($statement, 'si', $questionsIDs, $numberOfQuestions);

/*
 * Execute the query and bind the 'answer' column to the variable
 * $column. If more columns are retrieved you would need to add more
 * variables to bind to.
 */
mysqli_stmt_execute($statement);
mysqli_stmt_bind_result($statement, $column);

while(mysqli_stmt_fetch($statement)) {
    $correct[] = $column;
}

mysqli_stmt_close($statement);


/*
 * Calculate the score using a loop
 */
foreach($correct as $index => $value) {

    if($value == $answers[$index]) {
        $score++;
        continue;
    }

}

echo 'Your score: ' . $score;

I hope this can be of use to you. I also hope you understand the code and can see advantages before using it yourself.

Happy coding.

Answer 4

Time to learn loops/arrays!

The code you have and the problem you're encountering, is one of the strong pro's of why [an array][1] exists. They might be a bit difficult to understand, but as you will see below, it'll save endless lines of code. The DRY-technique (Don't repeat yourself) is a valuable skill.

---

This can be done a WHOLE lot simpeler if you change your htm so it will return an array to PHP:

<input name="answer2question[1]" />
<input name="answer2question[2]" />
<!-- ANY AMOUNT YOU WANT -->
<input name="answer2question[99]" />

On submitting, this will result in an array, $_POST['answer2question'] will have 3 values in the example above (with example values):

Array
(
    [1] => 314
    [2] => 42
    [99] => 34
)

---

Time to loop! Foreach()-time

We can loop though these values with an foreach():

$score = 0; // startscore
foreach($_POST['answer2question'] as $question_id => $given_answer){
    // Some VERY basic security, you should read up on SQL injection:
    $question_id  = mysqli_real_escape_string ($link, $question_id);
    $given_answer = mysqli_real_escape_string ($link, $given_answer);
    $sql = "SELECT id FROM `math` WHERE `bid` = '".$question_id."' AND answer='".$given_answer."' LIMIT 1";
    $query = mysqli_query($link,$sql);
    if( $query->num_rows!==0 ){
        $score += 1;
    }
}
echo $score;

• No hardcoded limit to the amount of questions, if you add another, the loop simple does another
• I don't fetch the data. This isn't needed, all we want to know if that question has that answer
  • This will improve performance! MySQL is a lot faster for comparing.
  • I've added a LIMIT 1. means "Stop looking for more results after 1st match" (performance up)
  • I only select 1 column, * selects all values to memory, you don't need that (performance up)

I don't know why you have the false's, So I kept them out. My code does the same as yours.

---

You fetch_array(). You don't want that, either use fetch_row() and use the $fetch array by integer (eg: $fetch[0]) or fetch_assoc() and access it by strings (eg: $fetch['example']), I recommend the latter.
This is better because fetch_array() defaults to doing both, so all your arrays have 2 entries for 1 value ([0] and ['example']).