8
\$\begingroup\$

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;
} 
\$\endgroup\$
  • \$\begingroup\$ everytime you have something like $answerChoice1 = $_POST['test1']; you can replace it by something like $answerChoice[$i] = $_POST['test' + $i]; and wrap that line in a loop to replace all ten identical lines. (basically. you may need to add the declarations of the arrays) \$\endgroup\$ – njzk2 Jun 18 '15 at 16:04
8
\$\begingroup\$

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();
\$\endgroup\$
  • \$\begingroup\$ The TS using mysqli, not PDO :) \$\endgroup\$ – Martijn Jun 18 '15 at 14:22
  • \$\begingroup\$ Thanks for the answer...can you please clarify how your version keeps track of score? And also, why do you have questions marks for the sql array?"The resulting string will look like 'SELECT COUNT(bid) FROM math WHERE (bid=? AND aanswer=?) OR (bid=? AND aanswer=?) OR ...'" \$\endgroup\$ – Nicholas Roberts Jun 18 '15 at 17:45
  • 1
    \$\begingroup\$ @NicholasRoberts The question marks will be replaced by contents of $params, this is a very helpful feature of the PDO library and is part of how your application can be kept safe from SQL injection. Documentation for PDO is here: php.net/manual/en/intro.pdo.php \$\endgroup\$ – Josh Rumbut Jun 18 '15 at 19:12
  • \$\begingroup\$ @NicholasRoberts The score is calculated by the database. The COUNT function returns the total number of (not null) results from the query. By selecting results where the id and answer match those provided by the user, only correct answers are selected. As per the question marks, Josh Rumbut is correct, they are placeholders used for a paramatized query, a safer alternative to direct string concatenation or variable interpretation in the sql string \$\endgroup\$ – Steve Jun 19 '15 at 10:53
  • \$\begingroup\$ This is starting to make sense to me, however the 'bid' is the Id number of the question, they're simply numbers. I use the 'bid' to select the 'aanswer' from that corresponding row. So I'm just confused why we compare the 'bid' to the data in the radio button (test1,2,3,4,5..). \$\endgroup\$ – Nicholas Roberts Jun 20 '15 at 6:10
15
\$\begingroup\$

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 !
\$\endgroup\$
  • 1
    \$\begingroup\$ I figure using a loop or an array would maybe be a good start but it seems overwhelming. To answer your question, I think he did. \$\endgroup\$ – Mast Jun 18 '15 at 10:40
  • 1
    \$\begingroup\$ Oh yeah, didn't see that. I'll find a way to emphasize more the fact that the for loop is definitely the way to go. \$\endgroup\$ – Loufylouf Jun 18 '15 at 10:44
3
\$\begingroup\$

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.

\$\endgroup\$
  • \$\begingroup\$ Same question than for the code submitted by Quill : are you sure the order of the answers will match the order you get from the form ? More precisely, is there an implicit ORDER clause in the query, added by MySQL ? I would definitely fetch also the bid field, to make sure than even if there is an implicit ORDER, I can be sure to match the questions from the form to the questions from the table. \$\endgroup\$ – Loufylouf Jun 18 '15 at 12:42
  • 1
    \$\begingroup\$ If your original answer is bad, then just edit it to make it good. \$\endgroup\$ – 200_success Jun 18 '15 at 19:22
  • \$\begingroup\$ @200_success - You are right. I have updated the answer. \$\endgroup\$ – AnotherGuy Jun 18 '15 at 23:44
2
\$\begingroup\$

Time to learn loops/arrays!

The code you have, and the problem you're encountering, is one of the strong pro's of an array. There 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']).

\$\endgroup\$
  • \$\begingroup\$ I agree with everything, except the big hole you have there. If you create a field named answer2question['; select from_base64('<your virus base64 code>') into outfile '<file name here>'; -- ], you will have a new virus on your system, which can be executed in other way (e.g.: a PHP file placed inside the public_html folder). Try using prepared statements, which will increase your security and your performance GREATLY!!! \$\endgroup\$ – Ismael Miguel Jun 18 '15 at 15:17
  • \$\begingroup\$ Just for your curiosity, the query generated with that exact code would be SELECT id FROM `math` WHERE `bid` = ''; select from_base64('<your virus base64 code>') into outfile '<file name here>'; -- ' AND answer='".$given_answer."' LIMIT 1, which is basically the same as running SELECT id FROM `math` WHERE `bid` = ''; and then select from_base64('<your virus base64 code>') into outfile '<file name here>';. \$\endgroup\$ – Ismael Miguel Jun 18 '15 at 15:22
  • 1
    \$\begingroup\$ -1 please do not post vulnerable code as example of good code. It doesn't matter that your main focus was the concept of the foreach loop, this normalizes vulnerable code. You also miss out on the performance increase of preparing the query once and running it multiple times, but the security issues are really the important thing. \$\endgroup\$ – tim Jun 18 '15 at 15:48
  • \$\begingroup\$ I've added the real_escape_string() as basic protection. About the optimising, prepared statements for so few queries actually costs more performance because of the overhead. Prepared statement are great 50+ queries, not the few he does now :) \$\endgroup\$ – Martijn Jun 19 '15 at 8:04
  • 2
    \$\begingroup\$ With the addon: While it'll cost you performance, it might be worth it for the increased securty \$\endgroup\$ – Martijn Jun 19 '15 at 8:06

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.