patternphpModerate
Math quiz in PHP
Viewed 0 times
phpquizmath
Problem
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`Solution
That may sound a bit harsh, but have you ever heard of the for loop ?
More on the SQL side of your code :
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 !
Context
StackExchange Code Review Q#93956, answer score: 15
Revisions (0)
No revisions yet.