8
\$\begingroup\$

I'm currently working on a kind of PHP Quiz about mathematical calculations. We show a mathematical equation, and user need to input his answer.

Currently, this is what I have written:

echo '<div class="table-title">Super Quizz</div>';

echo '<div class="row text-center quizz">';

function addition($x, $y) { return $x + $y; }
function substraction($x, $y) { return $x - $y; }
function multiplication($x, $y) { return $x * $y; }
function division($x, $y) { return $x / $y; }

do {
    $firstNumber = rand(1, 10);
    $secondNumber = rand(1, 10);

    $operatorsCharacteres = array('+', '-', 'x', '/');
    $operatorsLetter = array('addition', 'substraction', 'multiplication',             'division');
    $operande = array_rand($operatorsLetter);
    $result = call_user_func_array($operatorsLetter[$operande],     array($firstNumber, $secondNumber));
}while($result < 0);


echo $firstNumber.' '.$operatorsCharacteres[$operande].' '.$secondNumber.' = <input type="text" name="answer" maxlength="3" class="input-quizz" >';
echo '<input type="hidden" value="'.$result.'">';

echo ' </div>';

echo '<div class="table-title"><a href="#" type="submit">I valid !</a></div>';
echo 'Result : '.$result;

If something is wrong, or not optimised, please, tell me, and teach me.

\$\endgroup\$
  • \$\begingroup\$ @Vogel612 : Thank you for edit it, but it seems you deleted my second question, about the best way to save user data. Is it voluntary ? \$\endgroup\$ – Jerem. Bartman. Jan 31 '15 at 20:55
  • 2
    \$\begingroup\$ Yes that is planned. Unfortunately chnaging what your code does is not in scope for codereview, which is why I removed the request for additional 'features' from your question. If you have further questions feel free to ask away in Code Review Chat. You can find me in the 2nd monitor ;) \$\endgroup\$ – Vogel612 Jan 31 '15 at 21:01
  • 1
    \$\begingroup\$ Hi @Jerem.Bartman., nice of you to accept my answer! In the meantime, I noticed chumkiu's answer, and it's much better than mine, so I suggest to accept that one instead! \$\endgroup\$ – janos Jan 31 '15 at 21:37
10
\$\begingroup\$

Naming

The naming of the arrays related to the operators is not so great:

  • In $operatorsLetter you have the names of functions that do some operation. $operatorFunctionNames or even $operatorFunctions would be a better name.
  • In $operatorsCharacteres you have the symbols of operations. These are normally not called characters. So $operatorSymbols would be a better name.
  • In $operande you have the key of a random operator. That's not an operand. In the operation 3 + 9, the operands are 3 and 9, but in your code it's "addition". So $operatorIndex would be a better name.

With these renames, the code is slightly easier to understand:

$operatorSymbols = array('+', '-', 'x', '/');
$operatorFunctions = array('addition', 'substraction', 'multiplication', 'division');
$operatorIndex = array_rand($operatorFunctions);
$operatorSymbol = $operatorSymbols[$operatorIndex];
$operatorFunction = $operatorFunctions[$operatorIndex];
$result = call_user_func_array($operatorFunction, array($firstNumber, $secondNumber));

I also introduced the helper variables $operatorSymbol and $operatorFunction, to shorten the lines later in the code that use them.

Printing in PHP

An easier way to print in PHP is to simply move text outside of <?php ... ?> blocks. For example these lines at the beginning of your script are a bit tedious to write using echo statements:

echo '<div class="table-title">Super Quizz</div>';

echo '<div class="row text-center quizz">';

Instead, you could change your script to put the beginning <?php after those lines, like this:

<div class="table-title">Super Quizz</div>
<div class="row text-center quizz">

<?php
function addition($x, $y) { return $x + $y; }
function substraction($x, $y) { return $x - $y; }
// ...

I would do similarly near the end of the file too, like this:

// ...
echo $firstNumber.' '.$operatorSymbol.' '.$secondNumber;
?>
 = <input type="text" name="answer" maxlength="3" class="input-quizz" >
    <input type="hidden" value="<?php echo $result ?>">

</div>

<div class="table-title"><a href="#" type="submit">I valid !</a></div>
Result : <?php echo $result ?>
\$\endgroup\$
  • 1
    \$\begingroup\$ Don't use echo $firstNumber.' '.$operatorSymbol.' '.$secondNumber;, use echo $firstNumber,' ',$operatorSymbol,' ',$secondNumber;. This has a small performance gain. Since strings are immutable in PHP, a new string containing the contatenation of all those strings is created. And then echoed. With commas, you are sending multiple strings, which are sent directly to the default output mechanism. No more processing needed. No concatenation. It is a small performance optimization, but it's kinda worth it. It helps a lot in the future. \$\endgroup\$ – Ismael Miguel Feb 1 '15 at 3:31
10
\$\begingroup\$

I would:

  • improve (avoid) the PHP & HTML mixing.
  • use one associative array instead of two array in order to define the operators
  • avoid a while. If the problem is the substraction, then check if the result is < 0 and switch the variable. Because in your case the substraction case will be less probable than other operators.
  • call_user_func_array could be simply replaced legitimately by a variable ($funcName())

code

<?php
function addition($x, $y) { return $x + $y; }
function substraction($x, $y) { return $x - $y; }
function multiplication($x, $y) { return $x * $y; }
function division($x, $y) { return $x / $y; }

$firstNumber  = mt_rand(1, 10);
$secondNumber = mt_rand(1, 10);

$operators = array(
    'addition'       =>'+', 
    'substraction'   =>'-', 
    'multiplication' =>'x', 
    'division'       =>'/'
);

$operator = array_rand($operators);

$result = $operator($firstNumber, $secondNumber);

if($result < 0) {
    list($firstNumber,$secondNumber) = array($secondNumber, $firstNumber);
    $result *= -1;
}

?>
<div class="table-title">Super Quizz</div>
<div class="row text-center quizz">
    <?=$firstNumber?> <?=$operators[$operator]?> <?=$secondNumber?> = <input type="text" name="answer" maxlength="3" class="input-quizz" >
    <input type="hidden" value="<?=$result?>">
</div>
<div class="table-title"><a href="#" type="submit">I valid !</a></div>
Result : <?=$result?>

Just a note about <?=. This syntax could be disabled by .ini configuration if your PHP is <=5.3. In this case you must use the most verbose <?php echo

Another thing about rand. I've replaced automatically in my mind rand with mt_rand because this last "Generate a better random value". It's not so important in this context but it's a good thing start to use it for all.

\$\endgroup\$
  • \$\begingroup\$ Avoid using <?=$result?>. This can be disabled on his server/php installation. Use a proper <?php echo $result;?> for portability. \$\endgroup\$ – Ismael Miguel Feb 1 '15 at 3:57
  • \$\begingroup\$ Well, your comment is correct. However we talk about versions not yet supported (5.3 has supported until Aug 2014 and since 5.4 <?= is always available). So I prefer to leave it as answer, but insert a note for "vintage" installations \$\endgroup\$ – Luca Rainone Feb 1 '15 at 8:20
  • \$\begingroup\$ Thank you for your answer. But I don't understand some points. First, you write <?= but is it a correct way, if later, i want to add more calcul to the operation ? \$\endgroup\$ – Jerem. Bartman. Feb 1 '15 at 10:43
  • 1
    \$\begingroup\$ I'm not saying it isn't available. I'm saying that it can be disabled AND comes disabled by default on XAMPP. You should add this note. \$\endgroup\$ – Ismael Miguel Feb 1 '15 at 15:40
  • 1
    \$\begingroup\$ @Jerem.Bartman. <?= has no conflicts with anything, in any configuration, in any server IF you are under PHP5.4+ stackoverflow.com/questions/200640/… (see ending note). If you are under <= 5.3 then yes, it's better to you to use <?php echo. For other things: I can only review the code that you've posted. If you want to add more options, then it's better for you to post another question with your code approach :-) \$\endgroup\$ – Luca Rainone Feb 3 '15 at 12:19

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.