5
\$\begingroup\$

I would very much appreciate a general review of my PHP code which grabs information from a mySQL database and also inserts some information into the database.

Overview :

  1. Users go to /main.php and answer math questions. The 20 math questions are pulled from the database. The answers are pulled at the same time but they are not inserted into the html at this point.

  2. User answers math questions. If they leave any blank or it doesn't validate as a number they will receive an error with PHP.

  3. Once they entered all numbers and the token validation passes, I insert their answers into a row in my database for record. I also include the IP address and the cookie currently, but will likely add options for names at a later date.

  4. After the insert into database I redirect them to results.php where I echo their answers using the $_SESSION vars I saved earlier, and they can than match up their answers with the correct answers.

So would just like you to point out anything I'm doing that's dumb, stupid, incorrect, or just plain insane. As well as any other advice you have.

Lastly, and pretty importantly, is there any better and/or shorter way I can write my $addToPlayersAnswers statement? It's pretty long.

Example


main.php (where user answers the questions and then submits his/her answers)

<?php
session_start();
require '../abovepublic/file.php'; // just contains sql password, username, etc for $connect... it's above/outside the public html folder, is that good?
require 'someFunctionsAndStuff.php';

$requiredInputFields = ['token'];
for ($i = 1; $i < 21; $i += 1) {
    array_push($requiredInputFields, 'qora'.$i);
}

if (!empty($_POST)) {
    if (allFieldsFilled($requiredInputFields)) {
        if ($_SESSION['token'] === $_POST['token']) {
            if (veryifyAllIntegers()) {
                $iii = getUserIp();
                $iiii = session_id();
             // here I'm saving all the post vars as session so I can echo them on the 
              // redirect to the new page
               for ($yty = 1; $yty < 21; $yty += 1) {
                  $_SESSION['qora'.$yty] = $_POST['qora'.$yty];
               }
           // here  I'm inserting all the user's answers into the database
               $addToPlayersAnswers = "INSERT INTO PlayersAnswers(Cookie, IP, Answer1, Answer2, Answer3, Answer4, Answer5, Answer6, Answer7, Answer8, Answer9, Answer10, Answer11, Answer12, Answer13, Answer14, Answer15, Answer16, Answer17, Answer18, Answer19, Answer20) VALUES ('$iiii','$iii',".$_POST['qora1'].",".$_POST['qora2'].",".$_POST['qora3'].",".$_POST['qora4'].",".$_POST['qora5'].",".$_POST['qora6'].",".$_POST['qora7'].",".$_POST['qora8'].",".$_POST['qora9'].",".$_POST['qora10'].",".$_POST['qora11'].",".$_POST['qora12'].",".$_POST['qora13'].",".$_POST['qora14'].",".$_POST['qora15'].",".$_POST['qora16'].",".$_POST['qora17'].",".$_POST['qora18'].",".$_POST['qora19'].",".$_POST['qora20'].")";
                $QRYPlayersAnswers = mysqli_query($connect, $addToPlayersAnswers);

                header('Location: http://www.example.com/resultspage.php');
                exit;
            }
        }
        else {
            echo 'error 00045';
        }
    }
}

$token = crypt(microtime(true), '$5$rounds=1275$KJEJIOjslje82323ljsK234d$');
$_SESSION['token'] = $token;

?>
<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Math Game</title>
</head>
<body>
<form method="POST" name="theform">
<table>
<thead><tr><th>Questions</th><th>Your Answers</th><th>Correct?</th></tr></thead>
<tbody>
<?php HTMLform($theQuestions); ?>
</tbody>
</table>
<input name="token" type="hidden" value="<?php echo htmlspecialchars($token); ?>">
<input type="submit" value="DONE!" />
</form>
</body>
</html>

results.php (where user is redirected after they filled out the answers and form validation is correct)

<?php

session_start();

require '../abovepublic/file.php';
require 'someFunctionsAndStuff.php';

function HTMLformCompleted($theQuestions, $theAnswers) {
   $i = 1;
      foreach ($theQuestions as $theindex => $singlequestion) {
        echo '<tr><td><label for="qora' . $i . '">' . $singlequestion . '</label></td><td><input name="qora' . $i .  '" id="qora' . $i . '" type="text" value="'.$_SESSION["qora".$i].'"></td><td class="qorac qora' . $i . '">' . $theAnswers[$theindex] . '</td></tr>';
        $i += 1;
    }
}

?>

<!DOCTYPE html>
<html>
<head>
<meta charset="utf-8">
<title>Math Game</title>
</head>
<body>

<form>
<table>
<thead><tr><th>Questions</th><th>Your Answers</th><th>Correct?</th></tr></thead>
<tbody>
<?php HTMLformCompleted($theQuestions, $theAnswers);  ?>
</tbody>
</table>
</form>


</body>
</html>

someFunctionsAndStuff.php (includes some functions and stuff so it doesn't clutter up other files even more)

<?php

$theQuestions = array();
$theAnswers = array();

$STMTquestions = 'SELECT Question, Answer FROM QUESTIONS WHERE Number<21';
$QRYquestions = mysqli_query($connect, $STMTquestions);

while ($getQuestions = mysqli_fetch_array($QRYquestions, MYSQLI_ASSOC)) {
    array_push($theQuestions, $getQuestions['Question']);
    array_push($theAnswers, $getQuestions['Answer']);
}

function HTMLform($theQuestions) {
  $i = 1;
    foreach ($theQuestions as $singlequestion) {
        echo '<tr><td><label for="qora' . $i . '">' . $singlequestion . '</label></td><td><input name="qora' . $i . '" id="qora' . $i . '" type="text"></td><td class="qorac qora' . $i . '"></td></tr>';
        $i += 1;
    }
}

function verifyAllIntegers() {
  $displayError = false;
    for($i = 1; $i < 21; $i += 1) {
        $_POST['qora'.$i] = filter_var($_POST['qora'.$i], FILTER_SANITIZE_NUMBER_INT);
        if (!filter_var($_POST['qora'.$i], FILTER_VALIDATE_INT)) {
            $displayError = true;
        }
    }
    if ($displayError) {
       echo 'All answers must be valid numbers';
       return false;
    }
    else {
       return true;
    }
}

function allFieldsFilled($requiredInputFields) {
    $displayError = false;
       foreach($requiredInputFields as $field) {
           if (strlen(trim($_POST[$field])) < 1) {
               $displayError = true;
           }
       }
    if ($displayError) {
        echo "All questions must be answered";
        return false;
    }
    else {
        return true;
    }
}

function getUserIp() {
    if (!empty($_SERVER['HTTP_CLIENT_IP'])) {
        $userip = $_SERVER['HTTP_CLIENT_IP'];
    }
    elseif (!empty($_SERVER['HTTP_X_FORWARDED_FOR'])) {
        $userip = $_SERVER['HTTP_X_FORWARDED_FOR'];
    }
    else {
         $userip = $_SERVER['REMOTE_ADDR'];
    }
    return $userip;
}

?>
\$\endgroup\$
5
\$\begingroup\$

Starting from the top…

Common idioms for repeating a loop 20 times are:

for ($i = 0; $i < 20; $i++) {
    # Do something
}

or

for ($i = 1; $i <= 20; $i++) {
    # Do something
}

What you wrote, with 21, is something I'd consider weird and jarring. (The same comment applies to your SQL query — you should write <= 20 instead.)


allFieldsFilled() has a side-effect of displaying an error message. The side-effect in itself is a bad idea, but the fact that you call allFieldsFilled() before the HTML output even start means that you'll generate malformed HTML. It seems like you made your own trap and walked right into it.

Same goes for verifyAllIntegers().


$iii and $iiii are poor variable names.


Your table design, with one column for each of the 20 answers, is poor. As it is, the table is already unacceptably wide. What happens if you later want to change the program to ask 50 questions? That would necessitate a schema alteration. If you've ever worked in a company, you'll know that such operations are dreaded by developers and database administrators alike.

A reasonable schema might look something like this:

CREATE TABLE Quiz
( id INTEGER
, questionNumber INTEGER NOT NULL
, questionText VARCHAR(32) NOT NULL
, answerText NUMERIC NOT NULL
, PRIMARY KEY (id, questionNumber)
);

CREATE TABLE QuizAdministration
( id INTEGER
, quizId SERIAL PRIMARY KEY
, username VARCHAR(32) NOT NULL
, cookie VARCHAR(32) NOT NULL
, ip DECIMAL(39,0) NOT NULL
, time TIMESTAMP NOT NULL
, FOREIGN KEY (quizId) REFERENCES Quiz (id)
);

CREATE TABLE PlayerAnswers
( quizAdministrationId INTEGER NOT NULL
, questionNumber INTEGER NOT NULL
, answer NUMERIC
, FOREIGN KEY (quizAdministrationId) REFERENCES QuizAdministration (id)
);

Each time a player submits a quiz, insert one row into the QuizAdministration table and 20 dependent rows into PlayerAnswers.


Why are HTMLform() and HTMLformCompleted() defined in different places? They are very similar to each other. Similar enough that you might want to merge them into one function that works in two modes.

Composing the HTML for table rows using string concatenation makes me nervous about HTML injection attacks. Do you need to call htmlentities() to escape the question and answer text or not? If not, then you need to write a comment justifying why such escaping is not necessary.


Having global variables $theQuestions and $theAnswers defined via inclusion of someFunctionsAndStuff.php is surprising. Instead, the included file should define a function that returns those data.

\$\endgroup\$
  • \$\begingroup\$ No, creating a new table each time a person takes a quiz would be even worse than what you have now. I'll add more detail to the answer. \$\endgroup\$ – 200_success Mar 4 '14 at 10:54

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.