The attached file contains an "attempt.php" that, according to me, is much better structured than the current version. The main difference is found inside the 'foreach' loop that parses "input for question->answers" from the names ($key) of the posted parameters.
It looks like this:
foreach ($rawanswers as $key => $value) { // Parse input for question -> answers
if (substr($key, 0, 1) == "q") {
$key = substr($key,1);
if (isset($questions[$key])) { // It's a real question number, not a coded one
$questions[$key]->answer[] = trim($value);
} else if (substr_count($key, "rq")) { // Random Question information
$check = explode("rq", $key);
$key = $check[0]; // The random question id
$real = $check[1]; // The real question id
$questions[$key]->random = $real;
} else if (substr_count($key, "a")) { // Checkbox style multiple answers
$check = explode("a", $key);
$key = $check[0]; // The main question number
$value = $check[1]; // The actual answer
$questions[$key]->answer[] = trim($value);
} else if (substr_count($key, "r")) { // Random-style answers
$check = explode("r", $key);
$key = $check[0]; // The main question
$rand = $check[1]; // The random sub-question
$questions[$key]->answer[] = "$rand-$value";
} else {
error("Answer received for non-existent question ($key)!");
}
} else if ($key == "shuffleorder") {
$shuffleorder = explode(",", $value); // Actual order questions were given in
}
}
My suggestion for refactoring is
foreach ($rawanswers as $key => $value) { // Parse input for question -> answers
if (ereg('^q([0-9]+)$', $key, $keyregs)) { // It's a real question number, not a coded one
$questions[$keyregs[1]]->answer[] = trim($value);
} else if (ereg('^q([0-9]+)rq([0-9]+)$', $key, $keyregs)) { // Random Question information
$questions[$keyregs[1]]->random = $keyregs[2];
} else if (ereg('^q([0-9]+)a([0-9]+)$', $key, $keyregs)) { // Checkbox style multiple answers
$questions[$keyregs[1]]->answer[] = $keyregs[2];
} else if (ereg('^q([0-9]+)r([0-9]+)$', $key, $keyregs)) { // Random-style answers
$questions[$keyregs[1]]->answer[] = "$keyregs[2]-$value";
} else if ('shuffleorder' == $key) {
$shuffleorder = explode(",", $value); // Actual order questions were given in
}
}
My key argument for the refactoring is that my code would work fine also after removing all the 'else's:
foreach ($rawanswers as $key => $value) { // Parse input for question -> answers
if (ereg('^q([0-9]+)$', $key, $keyregs)) { // It's a real question number, not a coded one
$questions[$keyregs[1]]->answer[] = trim($value);
}
if (ereg('^q([0-9]+)rq([0-9]+)$', $key, $keyregs)) { // Random Question information
$questions[$keyregs[1]]->random = $keyregs[2];
}
if (ereg('^q([0-9]+)a([0-9]+)$', $key, $keyregs)) { // Checkbox style multiple answers
$questions[$keyregs[1]]->answer[] = $keyregs[2];
}
if (ereg('^q([0-9]+)r([0-9]+)$', $key, $keyregs)) { // Random-style answers
$questions[$keyregs[1]]->answer[] = "$keyregs[2]-$value";
}
if ('shuffleorder' == $key) {
$shuffleorder = explode(",", $value); // Actual order questions were given in
}
}
This works because each one of the if clauses evaluates true for the input parameter its body concern and false for all other input parameters. Why even consider a different structure? The reason for considering a different structure, like the current one, is probably that the developer was not aware of the power of using regular expressions and the function ereg.
The original structure is not only nested, it is also dependent on having (some of) the if-bodies in the right order, i.e. it would not work to put "Random-style answers" before "Random Question information". That and simular problems disappear in the refactoring. It is then much easier for a new developer to shoot in another if-ereg clause for any new attempt.php input parameter (that is what I am about to do) as the ereg expressions definetely define the parameter names that are already in use. You can then come down with your own exclusive input parameter pattern (like ^q[0-9]+ma[0-9]+$ ) and then add it as a separate 'else if' clause without worring about any possible name conflicts.
PS
I have kept the 'else's for performance reasons.
DS
