Исходный код
$counter_of_true_var=0;
print "<div style=\"padding:0 0 0 10\"><table cellpadding=\"0\" class=\"main_text\" border=\"0\" cellspacing=\"0\"><tr><td height=30 width=200><b>Правельные ответы</b></td><td><b>Ваши ответы</b></td><td></td></tr>";
for($i=1;$i<=20;$i++) {
$to_string = "hid"."$i";
$id_main = $FORM{$to_string};
$sth = $dbh->prepare("SELECT id, true_var from theme_questions where id=".$id_main." order by id asc");
$sth->execute;
$tr_var = $sth->fetchrow_array;
if ($i % 2) { $bgcolor="#ededed"; }
else { $bgcolor="#dcdcdc"; }
print "<tr><td height=\"20\" bgcolor=\"".$bgcolor."\">Вопрос № ".$i."). ".$tr_var."</td>";
$to_string_sub = "r"."$i";
if ($FORM{$to_string_sub} eq $tr_var) {
$counter_of_true_var++;
print "<td bgcolor=\"".$bgcolor."\">Вопрос №".$i.") ".$FORM{$to_string_sub}." </td><td style=\"padding:0 0 0 10\"><font color=\"green\">Правельный</font></td></tr>";
}
}Что не так в исходном коде
Запросы к БД внутри цикла
Так делать ни в коем случае не стоит. Правильно сначала собрать все идентификаторы в массив, сделать запрос через where id in (...), а потом запустить цикл по полученным из БД строкам.
Неудачный способ передачи массива через поля формы
Существует более оптимальный способ:
<select name="answers[5]">
<option value="1">10
<option value="2">20
<option value="3">30
</select>При таком названии поля мы получим ассоциативный массив, где ключом будет идентификатор вопроса, а значением — номер (или идентификатор) выбранного ответа.
Плотно перемешаны ответственности
Здесь наблюдаются как минимум три ответственности:
- прочитать правильные ответы из базы
- сформировать таблицу с выбранными и правильными ответами
- подсчитать количество правильных ответов
Причём эти ответственности переплетаются: строки, которые относятся к каждой из ответственностей, чередуются между собой.
Проще всего «расцепить» ответственности, перенеся их в отдельные методы. Однако в некоторых редких случаях достаточно будет просто перегруппировать строки кода.
Недочёты в именовании
- Переменная
$counter_of_true_varи полеtrue_varназваны неудачно: в них находится и подстчитывается не истинная переменная, а правильный ответ, right answer. - По названию переменных
$to_stringи$to_string_subсовершенно неясен их смысл. Похоже, это названия полей с идентификатором вопроса и с номером выбранного ответа. Лучше подойдут названия$id_field_nameи$answer_field_name. - В переменной
$tr_var, очевидно, содержится номер правильного ответа, то есть более удачным названием будет$right_answer. - Переменную
$FORMс натяжкой можно принять, хотя там всё-таки не форма, а значения полей формы. По-моему мнению, идеальным названием будет$submitted_answers(если допустить, что никакой другой информации вместе с выбранными ответами не поступает). - Название
$sthслишком короткое — не стоит так сильно экономить на символах в названиях. Экономия нажатий клавиш ничтожная, а ясность кода может снизиться заметно. - Имена полей формы также неудачны:
hidNиrN. Если уж выбран такой многословный способ передачи массива, то хотя бы названия полей должны быть говорящими:question_id_Nиchosen_answer_N. - Переменная
$id_mainне совсем точно называет содержащиеся в ней данные: на самом деле это идентификатор вопроса, то есть$question_id
Чтобы подбирать хорошие названия переменной или полю, стоит сначала по-русски проговорить смысл хранящихся в ней данных, и постараться сделать это максимально кратко, но так, чтобы эти данные нельзя было ни с чем перепутать.
Другие недочёты
Ещё ряд моментов, к которым есть вопросы.
Например, это способ вызывать процедуру и функцию через геттеры: $sth->execute и $sth->fetchrow_array. Геттеры — это способ получить информацию, которая добывается специфическим способом или не хранится в готовом виде. Второй вариант с fetchrow_array в глобальном смысле подходит под это определение, но тогда у геттера должно даваться название по смыслу данных, а не по операции. Вполне подойдёт next_row_array, но никакого глагола в названии геттера быть не должно.
Также к недочётам можно отнести:
- оператор
ifпри присвоении переменной$bgcolor(правильнее в таких ситуациях применять тернарный оператор?:) - конструкции
"hid"."$i"и"r"."$i" - использование конкатенации вместо подстановок:
"<tr><td height=\"20\" bgcolor=\"".$bgcolor."\">Вопрос № ".$i."). ".$tr_var."</td>" - использование инлайновых стилей
style=\"padding:0 0 0 10\", атрибутов и тегов<font>,<b>вместо классов таблицы стилей, хотя в целом они CSS имеет место - отсутствие защиты от SQL-инъекций
- раскраску строк через явное указание цвета: сейчас это стоит делать через псевдо-селекторы таблицы стилей
- отсутствие закрытия тега
<tr>в случае неправильного ответа
Вариант рефакторинга исходного кода
$question_ids = array_map(function ($id) { return (int)$id; }, array_keys($submitted_answers));
$questions_query = $dbh->prepare("SELECT id, question_text, right_answer from theme_questions where id IN (" . implode(',', $question_ids) . ") order by id asc");
$questions_query->execute;
$questions = $questions_query->all_rows_array; <em>// пусть такой метод будет, для простоты примера</em>?>
<table class="right-answers">
<thead>
<tr>
<th>Вопрос</th>
<th>Правильный ответ</th>
<th>Ваш ответ</th>
<th></th>
</tr>
</thead>
<tbody>
<?php
foreach ($questions as $question): ?>
<tr>
<td class="question--text"><?php $question['question_text'] ?></td>
<td class="question--right-answer"><?php $question['right_answer'] ?></td>
<td class="question--submitted-answer"><?php $submitted_answers[$question['id']] ?></td>
<td class="question--answer-is-right"><?php $submitted_answers[$question['id']] === $question['right_answer'] ? 'Правильный' : '' ?></td>
</tr>
<?php endforeach
$right_answer_questions = array_filter($questions, function ($question) {
return$submitted_answers[$question['id']] === $question['right_answer'];
});
$right_answers_count = count($right_answer_questions);