Что такое Code smell и как улучшить качество кода?

Часто, то как мы пишем код зависит от того как мы начинали программировать.

Например, если кто-то имеет образование в области информатики, то он скорее всего знает некоторые принципы, которые не позволят ему писать код плохого качества. Но это скорее наблюдение, чем правило. Соответственно, те, кто обучались программировать самостоятельно на различных ресурсах, могут имеет проблемы с качеством кода.

Внесу ясность: я не говорю, что формальное образование лучше, чем самообразование. Среди и тех и других есть выдающиеся программисты. Но и те и другие иногда выдают некачественный код.

В этой статье мы рассмотрим некоторые признаки плохого кода. В примерах будем использовать PHP.

В следующей статье рассмотрим как можно этого избежать. В частности, используя PHP, несколько инструментов и WordPress в качестве CMS.

Но сначала давайте разберемся какой код плохой.

Что такое Code smell?

Code smell - дословно переводится как "код с запашком". Википедия дает такое определение Code Smell:

Любые признаки в исходном коде программы, которые указывают на наличие глубоких проблем.

Отличное описание термина дает популярный программист Martin Flower. Вы можете прочитать его статью по ссылке, но я приведу здесь, по моему мнению, суть:

1. Code Smells - не сама проблема, а её индикатор;
2. Длинные методы это хороший пример плохого кода. (Имеется в виду, что необходимо разбивать большие методы на небольшие самостоятельные модули)
3. Еще пример - это классы, содержащие только данные, без поведения.

Если Вы не прочитали статью, то заканчивается она следующим:

Одна из особенностей code smells заключается в том, что неопытным людям легко их заметить, даже если знаний не достаточно, чтобы оценить реальную угрозу.

В общем, опыт не играет здесь ключевой роли.

А если ли примеры?

С учетом сказанного, почему бы не взглянуть на какой-то пример кода? Сначала рассмотрим проблемный код, а затем предложим вариант решения проблемы.

Пример 1. Соглашение об именовании

Один из примеров, которые легче всего увидеть - имена переменных, неясно отражающие содержимое. Когда из контекста не ясно, что должна хранить переменная.

Конечно, бывают случаи, когда это приемлемо (как i в цикле for). Но в более длинных методах это не совсем справедливо, как с циклом. Например:

public function get_things( $x ) {
 
  $l = array();
 
  for ( $i = 0; $i < count( $x ); $i++ ) {
 
    if ( true === $x[ $i ] ) {
      array_push( $l, $x[ $i ] );
    }
  }
  return $l;
}

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

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

С учетом этого можем переписать код:

public function get_flagged_items( $items ) {
 
  $flagged_items = array();
 
  for ( $i = 0; $i < count( $items ); $i++ ) {
 
    $current_item = $items[ $i ] ;
 
    if ( true === $current_item ) {
      array_push( $flagged_items, $current_item );
    }
  }
  return $flagged_items;
}

Код стал намного легче для понимания, не так ли? И обратите внимание, что алгоритм не изменился. Зато он стал более читаемым. Если спросить что делает этот код, то можно ответить:

Возвращает массив элементов помеченных как true из переданного массива элементов.

Насколько это возможно, избегайте использования общих имен переменных.

Пример 2. Оставайтесь "сухим"

В программировании сложно найти человека, который ни разу не слышал бы о подходах "KISS" (Keep it simple, stupid) и "DRY" (don't repeat yourself). Первый по-русски звучит как "чем проще - тем лучше", то есть большинство систем будут работать лучше, если их упростить. А второй говорит о том, что лучше переиспользовать по максимуму написанный код, не повторяя его у других частях программы.

Есть много способов продемонстрировать эти принципы, но приведу один, как делать не надо и как это можно исправить.

Предположим, что у нас есть функция, принимающая id записи и её заголовок. Code smell будет выглядеть так:

public function save_posts() {
 
  save_post( 1, 'Hello World!' );
  save_post( 2, 'Goodbye World!' );
  save_post( 3, 'What is this new world?' );
 
}

Но зачем нужно вызывать save_post() трижды? Вместо этого лучше задать ассоциативный массив и вызвать save_post() в цикле:

public function save_posts() {
 
  $posts = [
    1 => 'Hello World!',
    2 => 'Goodbye World!',
    3 => 'What is this new world?',
  ];
 
  foreach ( $posts as $post_id => $post_title ) {
    save_post( $post_id, $post_title );
  }
}

Таким образом, мы можем сделать метод еще более гибким, если добавим возможность передавать массив в качестве аргумента. При этом цикл foreach останется неизменным, независимо от количества элементов в переданном массиве.

Если Вы видите, что один и тот же метод вызывается в функции несколько раз с разными параметрами - это признак code smell. Уверен, что Вы найдете способы рефакторинга, чтобы вызвать метод только один раз.

Пример 3. Большое количество передаваемых параметров

Обычное проблема, которую мы можем часто наблюдать, не зависимо от языка программирования, это функции, принимающие большое количество параметров. Существует много мнений относительно того, сколько параметров передавать оптимально. Я думаю три (плюс минус два) это оптимальное количество.

Во-первых, давайте посмотрим как выглядит такая функция

public function submit_order( $first, $last, $address1, $address2, $city, $state, $zip, $phone, $cc, $expiration ) {
  /* Attempt to submit the order. If the order is successful,
   * then return an instance of an Order object with the success status;
   * otherwise, return an instance of an Order object with the failed status 
   * and a message.
   */
}

Нужно заботиться о том, сколько параметров требуется передать. Это делает вызов метода уродливым. При этом нужно еще реализовать внутри проверку передаваемых параметров.

Как это пофиксить? Лично я являюсь поклонником ООП для представления таких наборов информации. Для этого конкретного примера у нас может быть класс, который представляет контактную информацию человека. Кроме того, этот человек может быть связан с номером кредитной карты. Отделить данные от логики можно таким способом:

class Contact_Information {
  /* Maintains attributes of the person's contact information. */
}
 
class Payment_Information {
  /* Maintains the credit card number and other information for a person. */
}
 
class Order {
 
  public function submit( $contact_info, $payment_info ) {
 
    /* Attempt to submit the order. If the order is successful,
     * then return an instance of an Order object with the success status;
     * otherwise, return an instance of an Order object with the failed status 
     * and a message.
     */
  }
}

Этот небольшой рефакторинг является самым важным в этой статье. Обратите внимание, что мы сделали следующее:

1. Создали класс Contact_Information который позволяет инстанцировать объект включающий всю контактную информацию;
2. Создали класс Payment_Information который хранит информацию о кредитных картах и информацию, связанную с этими методами оплаты;
3. Создали класс Order с методом submit, отправляющим оплату. Теперь мы можем передавать в него только два параметра $contact_info и $payment_info

Надо понимать, что этот пример не умеет обрабатывать проверку соответствия платежной и контактной информации, не включает классы, которые могут быть необходимы для обработки неудачных попыток и пр.

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

Вывод

Список примеров далеко не полный, но эти ошибки довольно распространенные. Кроме того, есть довольно много ресурсов для идентификации и исправления code smells. К счастью, у нас также есть ряд инструментов, помогающих автоматических обнаружить их и очистить.