Рефакторинг: выделяй метод, когда это имеет смысл

Original author: Elias Fofanov
  • Translation
Сейчас уже сложно вспомнить тот момент, когда я впервые осознал, что выделять функции из больших кусков полезного кода, вообще-то, хорошая идея. То ли я получил это знание из “Совершенного кода”, то ли из “Чистого кода” — сложно вспомнить. В целом, это не особенно важно. Мы все знаем, что должны разносить бизнес-логику по хорошо проименованным функциям. Самая длинная функция, которую я когда-либо видео в жизни была длиной в 5к строк. Я лично знаком с тем “программистом”, что написал тот код. Помню, как впервые встретил эту функцию. Не сложно предсказать, что моей первой реакцией было: “Какого чёрта!!! Кто произвёл на свет этот кусок дерьма???”
Да, представьте себе, этот “программист” до сих пор слоняется тут в офисе, где я сейчас работаю над текущими проектами. Не хочу углубляться в эту историю, но хочу упомянуть, что та функция длиной в 5к строк была ядром программы, размером примерно в 150к строк. Разработка программы в конце концов зашла в тупик, из-за той ужасной функции, которая крайне негативно влияла на архитектуру приложения. В конце концов было принято решение о переписывании приложения с нуля.

Эта история иллюстрирует одну крайность проблемы размера функций, которая привела к плачевным последствиям. Другая крайность — отключить мозг и начать выделять повсюду классы с однострочными функциями внутри. Я не имею ввиду, что такие функции это плохо, я говорю о том, что не стоит забывать использовать мощности своего мозга. Вначале следует проаназировать проблему.
Перед тем, как я продолжу исследовать проблему глубже, хотел бы отметить, что, вообще говоря, некоторое время назад произошла небольшая баталия между Дядей Бобом и Кристин Горман на данную тему. Дядя Боб представил технику, которую назвал “Extract till you drop”, которая вкратце означает — извлекай функции до тех пор, пока есть что извлекать. Кристин Горман сочла, что эта техника исключает использование мозга. Вдобавок, был пост Джона Сонмеза насчёт рефакторинга одной функции из .NET BCL (хотя, изначальной целью статьи было показать, что большая часть комментов — зло).

Давайте рассмотрим пример рефакторинга, проведённого Джоном. Он взял для примера следующий метод:

internal static void SplitDirectoryFile(
     string path, out string directory, out string file)
{
    directory = null;
    file = null;

    // assumes a validated full path
    if (path != null)
    {
        int length = path.Length;
        int rootLength = GetRootLength(path);

        // ignore a trailing slash
        if (length > rootLength && EndsInDirectorySeparator(path))
            length--;

        // find the pivot index between end of string and root
        for (int pivot = length - 1; pivot >= rootLength; pivot--)
        {
            if (IsDirectorySeparator(path[pivot]))
            {
                directory = path.Substring(0, pivot);
                file = path.Substring(pivot + 1, length - pivot - 1);
                return;
            }
        }

        // no pivot, return just the trimmed directory
        directory = path.Substring(0, length);
    }
    return;
}


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

public class DirectoryFileSplitter
{
    private readonly string validatedFullPath;
    private int length;
    private int rootLength;
    private bool pivotFound;
    public string Directory { get; set; }
    public string File { get; set; }

    public DirectoryFileSplitter(string validatedFullPath)
    {
        this.validatedFullPath = validatedFullPath;
        length = validatedFullPath.Length;
        rootLength = GetRootLength(validatedFullPath);
    }

    public void Split()
    {
        if (validatedFullPath != null)
        {
            IgnoreTrailingSlash();

            FindPivotIndexBetweenEndOfStringAndRoot();

            if(!pivotFound)
                TrimDirectory();
        }
    }

    private void TrimDirectory()
    {
        Directory = validatedFullPath.Substring(0, length);
    }

    private void FindPivotIndexBetweenEndOfStringAndRoot()
    {
        for (int pivot = length - 1; pivot >= rootLength; pivot--)
        {
            if (IsDirectorySeparator(validatedFullPath[pivot]))
            {
                Directory = validatedFullPath.Substring(0, pivot);
                File = validatedFullPath.Substring(pivot + 1, length - pivot - 1);
                pivotFound = true;
            }
        }
    }

    private void IgnoreTrailingSlash()
    {
        if (length > rootLength && EndsInDirectorySeparator(validatedFullPath))
            length--;
    }
}


Вау, да? Не так уж и просто решить действительно ли рефакторинг помог сделать код более читабельным. Ощущение, что, вообще-то, его стало сложнее читать. Ранее была относительно небольшая функция с полезными комментариями, которая теперь была превращена в класс с четырьмя функциями внутри без комментариев. Я бы не сказал, что новый класс плох и весь рефакторинг был плохой идеей, а программиста, проделавшего рефакторинг следует казнить. Совсем нет. Я не настолько кровожаден. Между этими двумя примерами кода есть несколько отличий. Рассмотрим эти отличия:

  1. Если вы пытаетесь достичь глубокого понимания того, что делает функция верхнего уровня, от функция стала более трудной для чтения, чем была изначально, поскольку теперь нужно проскакать по всем функциям и понять, что происходит в каждой из них. Напротив, первоначальный вариант легко можно быстро пробежать глазами.
  2. Если вы пытаетесь понять, что делает функция верхнего уровня концептуально, то отрефакторенный вариант проще для чтения, поскольку мы сразу видим, что функция концептуально делает внутри себя.
  3. Третье отличие, которое я вижу — стоимость поддержки. Что касается нашего конкретного примера, то я бы сказал, что стоимость поддержки отрефакторенной версии выше, чем исходной (вам как минимум надо провести рефакторинг). В целом, ответ на вопрос о том, какой вариант более дорог в поддержке лежит в плоскости требований. Эти требования диктуют нам, важно ли в данной ситуации следовать SRP (принцип единственной ответственности) или нет. Если эта функция может быть единожды написана и забыта до скончания времён, то нет никаких причин для того, чтобы тратить время на её рефакторинг. Напротив, если ожидается, что функциональность будет расти, тогда у вас есть все причины для того, чтобы отрефакторить функцию в отдельный класс.

Вдобавок хочу коснуться ситуации, когда вы случайно (или преднамеренно) натыкаетесь на подобную функцию в легаси-системе. Вы сразу рванёте извлекать класс с четырьмя функциями внутри? Мой совет — не делайте этого без каких-либо на то причин, даже если покрытие тестами вашей кодовой базы стремится к 100%. Почему? Потому что тут нет никакого технического долга. Я говорю о серьёзном техническом долге, который является причиной страданий.

Таким образом, нет ничего плохого в технике “extract till you drop”. Вы просто должны держать в голове кое-какие соображения, по моему мнению.
Подытожив, хочу сказать, что вы никогда не должны совершать бессмысленных поступков. Вы должны сначала подумать, проанализировать, сделать заключение и только после этого действовать.
Ads
AdBlock has stolen the banner, but banners are not teeth — they will be back

More

Comments 17

    0
    OFF: какой смысл в извлечении мозга?
      +1
      > Включи мозг и извлекай, когда это имеет смысл
      ага, название статьи какое-то дебильно-дебильное: «включи мозг»-«имеет смысл», а что извлекай… пока не прочтешь, думаешь «сам то мозг включил?»
      уж лучше "Рефакторинг: выделяй метод, когда это имеет смысл"
        0
        Спасибо. Поменял название.
        +1
        Я бы сократил: «Рефакторинг — выделяй смысл» :-)
        +2
        На мой взгляд тут нужно менять интерфейс. Поделив функцию на две: извлечение директории и извлечение файла.
        Что касается отрефакторенного класса — лично меня напрягают классы которые что-то делают а потом сохраняют результат в себя чтобы вы могли достать его из свойства. На мой взгляд если вы хотите вернуть два и более значения — делите их обработку на разные публичные методы либо возвращайте структуру.
          +1
          Более того. Как выглядело использование функции до рефакторинга? Вызвал функцию одной строчкой, и все. После рефакторинга — надо написать 3 строчки (если заметите ошибки в синтаксисе — простите, я джавист):

          DirectoryFileSplitter ds = new DirectoryFileSplitter(myPath);
          ds.Split() // почему бы не вызывать это в конструкторе?
          string myLength = ds.length;

          Ну так вот, мне приходилось пользоваться API, в котором надо выполнить 3 строчки для одного действия. Это такая боль, скажу я вам. То split() забудешь, то в первой и третьей строчке по невнимательности используешь разные переменные.

          Теперь представьте, что надо реализовать сложную логику, где фигурируют 100500 разных директорий. Отладка станет болью и мучением. В результате вы напишете обертку, которая позволит вместо 3 строчек писать одну, и это будет как глоток свежего воздуха.
            0
            Абсолютно согласен.
            Хочу добавить, что тут придется создавать объект, под который будет выделяться память в managed heap, что есть не очень хорошо.
            Да и если логически подумать, это все ООП головного мозга, в BCL, например, ведь нет классов IntParser или NumberSummer.
          –2
          --что та функция длиной в 5к строк

          А в чем проблема с раземрами? В сервере одной из компаний в которой я работал были функции по 10К строк и тело класса находилось в файле размером почти мегабайт. (Прошлось просить компанию Perforce поправить их продукт чтобы работал с файлами таког размера).

          Вроде никто неудобства не испытывал.
            +4
            Сам факт того, что пришлось просить компанию Perforce о многом уже говорит.
            • UFO just landed and posted this here
              +8
              Сам C# много лет уже не использовал, поэтому где-то могу ошибаться. Все мои советы нацелены на читаемость и понятность, а не на быстродействие.

              1) Неряшливый код что первого примера, что второго, не позволяет нормально относиться ни к одному из них.

              2) «out» параметры довольно опасны и неочевидны. Если надо вернуть несколько параметров, то можно вернуть структуру или tuple (в C# tuples вроде давно есть уже).

              3) Сама функция сложно воспринимается из-за if-условий для валидации. Проваленные валидации должны выходить нафиг из функции или кидать исключение:
              // Так лучше не делать.
              if (path != null) {
                  // Тут весь оставшийся метод.
              }
              
              // Так будет понятнее, т.к. валидация логически отделена от самого метода.
              if (path == null) { return; }
              
              // Тут весь оставшийся метод.
              

              Убирается лишний уровень отступа и функция выглядит проще.

              4) Манипуляции с length не очень красивы. Лучше делать какую-то нормализацию входных данных. Типа убрать trailing-слеши у всего и дальше о них не думать. Опять же нормализация данных не относиться непосредственно к логике метода и должна быть как-то отделена от него, см. пример с валидациями.

              5) В исходном методе 12 значимых строк. Делать ради этого класс мне кажется сильно круто, т.к. в итоге будет больше строк и сложность скорее увеличится, чем уменьшится. В С#, по-моему, если возможность добавления методов в уже существующие классы. Если эта функция повсеместно используется в коде, то ее вполне можно утащить в какой-нибудь класс типа Directory. Но это уже кто как любит.

              Вообще, красивый метод из и из 200 строк напрягать не будет, а ужасный метод и из 20 строк будет казаться супер сложным и непонятным. Поэтому основной совет всегда один: пишите красиво, а не как курица лапой.
                +1
                Вообще, красивый метод из и из 200 строк напрягать не будет, а ужасный метод и из 20 строк будет казаться супер сложным и непонятным.


                Не согласен. В книгах по алгоритмам, как правило, все методы не более чем на 20 строк, очень сжатые, использующие не всегда очевидные конструкции. Но за счет размера их гораздо проще понять, хотя придется читать каждую строчку, а не наискосок.
                  0
                  Видел я один раз алгоритм, который вычислял число «Пи».
                  Алгоритм был в 6 строк кода, использовал около 8 переменных, одна из которых — массив.
                  Понять, что он делает, было совершенно нереально, пока не запустил и не увидел результат.
                  Но это простой случай.
                  А теперь представим, что вместо операций над переменными используются операции над интерфейсами внешних систем (той же OpenGL, например).

                  Разберетесь ли Вы в том, что делают эти 6 строк кода?

                  Правильнее даже задать вопрос по другому.

                  Разберетесь ли Вы, что делает эта функция? Каков ее смысл?
                  IMHO, в большинстве случаев Вам придется изучить несколько мест использования этой функции, чтобы это понять.
                –1
                «DirectoryFileSplitter». Напомните, чей это совет не давать классам имена, оканчивающиеся на «er»?

                По-хорошему, если уж выделять класс, то это должен быть класс Path со свойствами Directory и File, в конструкторе которого и делалась бы вся работа.
                  +1
                  Не слышал о таком совете. А есть такой?)
                    0
                    Вот хорошая вводная статья со ссылочками на более фундаментальные.

                    Дело, конечно же, не в буковках имени. Смысл рекомендации в том, что следует избавляться от классов-сервисов и рассматривать все классы как сущности.
                    0
                    По-моему, один из них был Мартин Фаулер.

                  Only users with full accounts can post comments. Log in, please.