Пример инлайнового разделения ответственностей в коде рендеринга таблицы правильных ответов


Исходный код

$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>

При таком названии поля мы получим ассоциативный массив, где ключом будет идентификатор вопроса, а значением — номер (или идентификатор) выбранного ответа.

Плотно перемешаны ответственности

Здесь наблюдаются как минимум три ответственности:

  • прочитать правильные ответы из базы
  • сформировать таблицу с выбранными и правильными ответами
  • подсчитать количество правильных ответов

Причём эти ответственности переплетаются: строки, которые относятся к каждой из ответственностей, чередуются между собой.

Проще всего «расцепить» ответственности, перенеся их в отдельные методы. Однако в некоторых редких случаях достаточно будет просто перегруппировать строки кода.

Недочёты в именовании

  1. Переменная $counter_of_true_var и поле true_var названы неудачно: в них находится и подстчитывается не истинная переменная, а правильный ответ, right answer.
  2. По названию переменных $to_string и $to_string_sub совершенно неясен их смысл. Похоже, это названия полей с идентификатором вопроса и с номером выбранного ответа. Лучше подойдут названия $id_field_name и $answer_field_name.
  3. В переменной $tr_var, очевидно, содержится номер правильного ответа, то есть более удачным названием будет $right_answer.
  4. Переменную $FORM с натяжкой можно принять, хотя там всё-таки не форма, а значения полей формы. По-моему мнению, идеальным названием будет $submitted_answers (если допустить, что никакой другой информации вместе с выбранными ответами не поступает).
  5. Название $sth слишком короткое — не стоит так сильно экономить на символах в названиях. Экономия нажатий клавиш ничтожная, а ясность кода может снизиться заметно.
  6. Имена полей формы также неудачны: hidN и rN. Если уж выбран такой многословный способ передачи массива, то хотя бы названия полей должны быть говорящими: question_id_N и chosen_answer_N.
  7. Переменная $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);

Теория