Да, представьте себе, этот “программист” до сих пор слоняется тут в офисе, где я сейчас работаю над текущими проектами. Не хочу углубляться в эту историю, но хочу упомянуть, что та функция длиной в 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--;
}
}
Вау, да? Не так уж и просто решить действительно ли рефакторинг помог сделать код более читабельным. Ощущение, что, вообще-то, его стало сложнее читать. Ранее была относительно небольшая функция с полезными комментариями, которая теперь была превращена в класс с четырьмя функциями внутри без комментариев. Я бы не сказал, что новый класс плох и весь рефакторинг был плохой идеей, а программиста, проделавшего рефакторинг следует казнить. Совсем нет. Я не настолько кровожаден. Между этими двумя примерами кода есть несколько отличий. Рассмотрим эти отличия:
- Если вы пытаетесь достичь глубокого понимания того, что делает функция верхнего уровня, от функция стала более трудной для чтения, чем была изначально, поскольку теперь нужно проскакать по всем функциям и понять, что происходит в каждой из них. Напротив, первоначальный вариант легко можно быстро пробежать глазами.
- Если вы пытаетесь понять, что делает функция верхнего уровня концептуально, то отрефакторенный вариант проще для чтения, поскольку мы сразу видим, что функция концептуально делает внутри себя.
- Третье отличие, которое я вижу — стоимость поддержки. Что касается нашего конкретного примера, то я бы сказал, что стоимость поддержки отрефакторенной версии выше, чем исходной (вам как минимум надо провести рефакторинг). В целом, ответ на вопрос о том, какой вариант более дорог в поддержке лежит в плоскости требований. Эти требования диктуют нам, важно ли в данной ситуации следовать SRP (принцип единственной ответственности) или нет. Если эта функция может быть единожды написана и забыта до скончания времён, то нет никаких причин для того, чтобы тратить время на её рефакторинг. Напротив, если ожидается, что функциональность будет расти, тогда у вас есть все причины для того, чтобы отрефакторить функцию в отдельный класс.
Вдобавок хочу коснуться ситуации, когда вы случайно (или преднамеренно) натыкаетесь на подобную функцию в легаси-системе. Вы сразу рванёте извлекать класс с четырьмя функциями внутри? Мой совет — не делайте этого без каких-либо на то причин, даже если покрытие тестами вашей кодовой базы стремится к 100%. Почему? Потому что тут нет никакого технического долга. Я говорю о серьёзном техническом долге, который является причиной страданий.
Таким образом, нет ничего плохого в технике “extract till you drop”. Вы просто должны держать в голове кое-какие соображения, по моему мнению.
Подытожив, хочу сказать, что вы никогда не должны совершать бессмысленных поступков. Вы должны сначала подумать, проанализировать, сделать заключение и только после этого действовать.
Комментарии (14)
DoctorX
23.07.2015 17:38+2На мой взгляд тут нужно менять интерфейс. Поделив функцию на две: извлечение директории и извлечение файла.
Что касается отрефакторенного класса — лично меня напрягают классы которые что-то делают а потом сохраняют результат в себя чтобы вы могли достать его из свойства. На мой взгляд если вы хотите вернуть два и более значения — делите их обработку на разные публичные методы либо возвращайте структуру.
Oleg_Sh
23.07.2015 18:43+1Более того. Как выглядело использование функции до рефакторинга? Вызвал функцию одной строчкой, и все. После рефакторинга — надо написать 3 строчки (если заметите ошибки в синтаксисе — простите, я джавист):
DirectoryFileSplitter ds = new DirectoryFileSplitter(myPath);
ds.Split() // почему бы не вызывать это в конструкторе?
string myLength = ds.length;
Ну так вот, мне приходилось пользоваться API, в котором надо выполнить 3 строчки для одного действия. Это такая боль, скажу я вам. То split() забудешь, то в первой и третьей строчке по невнимательности используешь разные переменные.
Теперь представьте, что надо реализовать сложную логику, где фигурируют 100500 разных директорий. Отладка станет болью и мучением. В результате вы напишете обертку, которая позволит вместо 3 строчек писать одну, и это будет как глоток свежего воздуха.
BloodUnit
23.07.2015 20:03Абсолютно согласен.
Хочу добавить, что тут придется создавать объект, под который будет выделяться память в managed heap, что есть не очень хорошо.
Да и если логически подумать, это все ООП головного мозга, в BCL, например, ведь нет классов IntParser или NumberSummer.
BalinTomsk
23.07.2015 20:45-2--что та функция длиной в 5к строк
А в чем проблема с раземрами? В сервере одной из компаний в которой я работал были функции по 10К строк и тело класса находилось в файле размером почти мегабайт. (Прошлось просить компанию Perforce поправить их продукт чтобы работал с файлами таког размера).
Вроде никто неудобства не испытывал.Mixim333
24.07.2015 19:51На мой субъективный взгляд, функции\методы более 100 строк — это уже зло — сложно без скролла понять, что они делают и как. Сам в 90% случаев при начале реализации какой-то задачи достаю блокнот с листами A4, рисую схему, на ней выделяю шаги решения задачи и к этим шагам приписываю имена методов, которые мне понадобятся — как правило получаются неплохо структурированные классы с небольшими методами (разумеется, до того, как код отрефакторил Джон не доходит)
BlackFoks
23.07.2015 20:59+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 строк будет казаться супер сложным и непонятным. Поэтому основной совет всегда один: пишите красиво, а не как курица лапой.KAW
28.07.2015 14:31+1Вообще, красивый метод из и из 200 строк напрягать не будет, а ужасный метод и из 20 строк будет казаться супер сложным и непонятным.
Не согласен. В книгах по алгоритмам, как правило, все методы не более чем на 20 строк, очень сжатые, использующие не всегда очевидные конструкции. Но за счет размера их гораздо проще понять, хотя придется читать каждую строчку, а не наискосок.
sophist
24.07.2015 10:17-1«DirectoryFileSplitter». Напомните, чей это совет не давать классам имена, оканчивающиеся на «er»?
По-хорошему, если уж выделять класс, то это должен быть класс Path со свойствами Directory и File, в конструкторе которого и делалась бы вся работа.
Borz
OFF: какой смысл в извлечении мозга?
zelyony
> Включи мозг и извлекай, когда это имеет смысл
ага, название статьи какое-то дебильно-дебильное: «включи мозг»-«имеет смысл», а что извлекай… пока не прочтешь, думаешь «сам то мозг включил?»
уж лучше "Рефакторинг: выделяй метод, когда это имеет смысл"
EngineerSpock Автор
Спасибо. Поменял название.