Govnokodu.Net

Пример рефакторинга «извлечение метода» для кода выполнения SQL-запроса

Исходный код

function query_safe()
{
  $time_before = $this->get_real_time();

  if (!$this->connected) $this->connect(DBUSER, DBPASS, DBNAME, DBHOST);
  
  $args = func_get_args();
  $tmpl =& $args[0];
  $tmpl = str_replace("%", "%%", $tmpl);
  $tmpl = str_replace("?", "%s", $tmpl);
  foreach ($argsas$i=>$v) {
      if (!$i) continue;
      if (is_int($v)) continue;
      $args[$i] = "'".mysql_escape_string($v)."'";
  }
  for ($i=$c=count($args)-1; $i<$c+20; $i++)
      $args[$i+1] = "UNKNOWN_SQL_COMMAND_$i";
  
  $sql = call_user_func_array("sprintf", $args);
      
  if (!($this->query_id = mysql_query($sql, $this->db_id) )) {
    $this->mysql_error = mysql_error();
    $this->mysql_error_num = mysql_errno();
    if ($show_error) {
      $this->display_error($this->mysql_error, $this->mysql_error_num, $query);
    }
  }
  
  $this->MySQL_time_taken += $this->get_real_time() - $time_before;
  $this->query_num ++;
  return $this->query_id;
}

Что не так в исходном коде

Этот метод довольно длинный, одно это уже настораживает.

В самом начале метода я вижу подключение к базе данных, а потом какие-то пока неясные манипуляции со строками — это второй звоночек. Зачем подключаться к базе данных, если ты ещё не собираешься отправлять запрос?

Беглый осмотр оставшегося кода показывает, что здесь достигаются две независимые друг от друга цели: сформировать SQL-запрос из параметризованной строки и собственно выполнить готовый SQL-запрос. Связаны они только тем, что выход одной является входом другой, внутренние же особенности никак друг на друга не влияют.

Для меня представляется совершенно естественным, что у меня будут два отдельных метода: для получения окончательного запроса из шаблона с параметрами, и для выполнения полностью готового запроса. Первый может легко понадобиться, например, при отладке, а второй — когда у меня уже есть полностью готовый запрос.

Углубляясь в код подготовки SQL-запроса можно видеть, что там алгоритм тоже работает в несколько этапов:

  • подготовка шаблонной строки,
  • подготовка параметров,
  • обработка ошибочной ситуации с параметрами,
  • собственно подстановка параметров.

Аналогичная картина и с выполнением запроса:

  • подготовка к выполнению запроса,
  • собственно выполнение запроса,
  • обработка ошибок,
  • завершающие действия.

Все эти подробности нет большого смысла делать доступными извне, но есть смысл всё равно раскидать их по отдельным методам, чтобы изолировать друг от друга содержащиеся в них знания.

Вариант рефакторинга исходного кода

function query_safe()
{
  $raw_sql = call_user_func_array(array($this, 'make_sql_safe'), func_get_args());
  return $this->query_raw_sql($raw_sql);
}
function make_sql_safe()
{
  $sql_params = func_get_args();
  $sql_template = $this->prepare_sql_template(array_shift($sql_params));
  $sql_params = $this->prepare_sql_params($sql_params);
  $raw_sql = vsprintf($sql_template, $sql_params);
  return $raw_sql;
}

function query_raw_sql($raw_sql)
{
  $this->before_query();
  if (!($this->query_id = mysql_query($raw_sql, $this->db_id) )) {
    $this->handle_mysql_error($raw_sql);
  }
  $this->after_query();

  return $this->query_id;
}

const SQL_TEMPLATE_REPLACES = [
  '%' => '%%',
  '?' => '%s',
]
private function prepare_sql_template($sql_template)
{
  return strtr($sql_template, self::SQL_TEMPLATE_REPLACES);
}

private function prepare_sql_params($sql_params)
{
  foreach ($sql_paramsas$i => $v) {
      if (!$i) continue;
      if (is_int($v)) continue;
      $sql_params[$i] = "'".mysql_escape_string($v)."'";
  }
  for ($i = 0; $i < 20; $i++)
      $sql_params[] = "MISSING_PARAM";
  return $sql_params;
}

private function before_query()
{
  if (!$this->connected) $this->connect(DBUSER, DBPASS, DBNAME, DBHOST);
  $this->query_started_at = $this->get_real_time();
}

private function after_query()
{
  $query_time_taken = $this->get_real_time() - $this->query_started_at;
  $this->MySQL_time_taken += $query_time_taken;
  $this->query_num ++;
}

private function handle_mysql_error($raw_sql)
{
    $this->mysql_error = mysql_error();
    $this->mysql_error_num = mysql_errno();
    if ($this->show_error) {
      $this->display_error($this->mysql_error, $this->mysql_error_num, $raw_sql);
    }
}

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

Теория

Сергей Галанин