Исходный код
$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);