Рефакторим легаси при помощи ООП

• Несколько слов перед началом
• Баг
  • Проблема
  • Код
• Рефакторинг
  • Проблема
  • Наивный подход
  • Строитель
  • Решение
• Послесловие

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

Несколько слов перед началом

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

Причём в этот раз в качестве примера вместо статического анализа выступит более привычная веб-разработка (хоть и на Blazor).

Баг

Проблема

Я пытался понять, что там происходит, но я сдаюсь.

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

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

Этим проектом был модуль статистики. Сама проблема казалась тривиальной — в одной из таблиц значения двух столбцов (Activated и Answered) были перепутаны местами.

Кажется просто, да? Ну, давайте проверим.

Код

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

Следствие привело меня в статической метод длиной 500 строк. Вот, например, сигнатура этого метода:

private static (
    Dictionary<string, List<int>> Requests,
    // .... ещё 12 словарей
    Dictionary<DateTime,
        Dictionary<string, (int Count, int Activated, int Answered, 
           Dictionary<string,
             (int Count, int Activated, int Answered)> Medium)>> UTMs)
    GetMailDataForStatForPeriod<TMail>(
        List<TMail> mails,
        DateTime RightDate,
        int monthsIterations) where TMail : Mail
{

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

Дальше идёт объявление возвращаемых объектов:

var requests = new Dictionary<string, List<int>>
    {
        { requestName, new() }
    };

var requestsReadAnswered = new Dictionary<string, List<int>>
    {
        { requestName, new() },
        { "Read", new() }
    };

// и много больше

var UTMs = new Dictionary<DateTime,
  Dictionary<string, (int Count, int Activated, int Answered,
    Dictionary<string, (int Count, int Activated, int Answered)> Medium)>>();

После этого объявляются счётчики для каждого месяца:

for (int i = 0; i < monthsIterations; i++)
{
    int countReqeusts = 0;
    int countRead = 0;
    int countActivated = 0;
    int countAnswered = 0;

    // и много, много больше

    UTMs.Add(RightDate, new());
    // ....
}

И, в конце концов, запускается проход по всей выборке данных. При соответствии записи какому-то из условий инкрементируются соответствующие счётчики:

if (trial != null && trial.Activated)
{
    countActivated++;
    countRead++;
}
else if (chainGroup.Any(mail => mail.Read || mail.Ticket > 0))
{
    countRead++;
}

// и много, много, много больше

if (trial != null)
{
    // ....

    if (lastUTM != default)
        source = lastUTM.Source;

    if (!UTMs[RightDate].ContainsKey(source))
        UTMs[RightDate].Add(source, (0, 0, 0, new()));
    var currSource = UTMs[RightDate][source];

    (int Count, int Answered, int Activated) currMedium = (0, 0, 0);
    if (ans != null)
    {
        if (lastUTM != default)
            currMedium.Answered++;

        currSource.Answered++;
    }
    // ....
}

После того, как я попробовал вникнуть в этот код, почувствовал себя примерно как в этом меме. И отнюдь не в роли терминатора.

В какой-то момент я решил просто сдаться. Код было сложно читать из-за нагромождённости и однотипности (так выглядел весь класс на две тысячи строк), а отлаживать этот единый цикл совсем не хотелось. Поэтому задача отправилась к человеку, который лучше знаком с кодом. И он её сделал довольно быстро — проблема оказалась совершенно тривиальной. Ошибку, кстати, можно найти в коде выше.

Конечно, не нужно много думать, чтобы сразу обвинить во всём кортежи — на ООП языке пишем, в конце-то концов! Да что там, даже я, при всей своей любви к новшествам в языке, немного поник от такого их использования. Так что, когда баг исправили, на этом коде была оставлена зарубка, чтобы позже его основательно отрефакторить.

Рефакторинг

Проблема

Итак, у нас появилось время порефакторить. Как мы его потратим?

Для начала надо очертить конкретный список проблем, которые хочется решить:

• Избавиться от кортежей, чтобы подобных багов больше не было;
• Разложить сбор разных статистик по полочкам. Сейчас методы очень большие и состоят друг из друга, так как разным страницам нужны похожие, но отличные данные;
• Чтобы это не превратилось в простое разбиение одного класса на несколько, надо восстановить хотя бы видимость ООП.

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

А вот второй и третий можно обобщить как разбиение god метода статистики на ООП решение. И раз уж мы сели рефакторить, то этим и займёмся.

Наивный подход

Хочешь решить проблему — начни с самого простого. Что первое приходит в голову?

• Чтобы отрефакторить god метод, можно разбить его на методы поменьше, объединённые общей тематикой.
• В нашем случае выделить тематические методы просто, так как по датасету собирается разная информация: UTM-метки, активации, прочтения и многое другое, что не было упомянуто в коде выше.
• Все эти методы можно было бы вызывать в ещё одном методе сверху. Вот только это уже сделано, и ситуацию это не исправило. Методы всё ещё большие, и сейчас их сильно уменьшить не выйдет, т.к. количество copy-paste кода только вырастет. Думаем дальше.
• Можно сделать не один большой цикл, а много маленьких (маленький в отношении строк кода — датасет для обработки у нас один). Ещё и неуклюжий for можно будет заменить на опрятный LINQ.

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

Очевидно, в таком случае время загрузки увеличится примерно в ~k раз, где k — количество таких методов с циклом внутри. И в случае с хотя бы относительно большим набором данных (десятки тысяч записей) и какой-то их трансформацией по ходу анализа, эффект будет чувствоваться достаточно сильно. Да, O(n*k) это не так плохо, как O(n^k), но не оптимально.

Строитель

Наконец-то мы дошли до актуального решения. Итак, что мы имеем:

• сложную вереницу методов для сборки статистики, которые хочется разбить ещё и изнутри из-за того, что внутри они достаточно сложные;
• данные надо как-то обходить, причём обходить специфичным образом — по датам.

Достаём книжку GoF по паттернам и смотрим, что нам тут подходит. И бинго! Для сложной инициализации как раз создан строитель (builder), а для инкапсуляции обхода существует итератор. Про итератор уже в подробностях рассказывала предыдущая статья, так что в теоретической части мы его пропустим. А вот про строителя сейчас расскажем для тех читателей, которые с ним ещё не встречались, а остальные могут освежить свои знания.

Вот классическая UML-диаграмма строителя:

Действующие лица:

• Director (распорядитель) — объект, управляющий процессом строительства. Вызывает все BuildPart строителя.
• Builder — общий интерфейс для строителя, определяющий шаги построения объекта.
• ConcreteBuilder — конкретная реализация строителя, реализующая шаги построения объекта. Предоставляет интерфейс доступа к продукту (GetResult()).
• Product — итоговый объект, который мы конструируем.

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

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

Определим сущности, с которыми будем работать:

• В качестве распорядителя определим ChartFactory — простую фабрику, которая будет руководить процессом построения.
• Конкретные реализации будет обобщать ChartBuilder — абстрактный класс с общей логикой и интерфейсом для реализации внутри.
• Конкретными реализациями являются PieChartBuilder и ColumnChartBuilder. Первый имеет настройку отображения процентов, а второй — режим торнадо-диаграммы.
• Продуктом будет являться Image — класс, определённый в подключённой нами библиотеке. Нужен для генерации картинки.

UML-диаграмма будет выглядеть так:

В этом случае можно всё же заметить одно отличие от диаграммы выше: здесь GetChart определён в родительском интерфейсе, хотя там он отдавался на откуп конкретным реализациям. Аргументация составителей паттернов состоит в том, что строители могут делать принципиально разные объекты, и выводить общий интерфейс для них не обязательно. Но так как у нас такой интерфейс уже есть, большого вреда от маленького шага в сторону не будет.

Дело за малым — просто написать код. ChartBuilder будет выглядеть так:

public abstract class ChartBuilder
{
    private int width = Consts.DefaultChartWidth;
    private int Height = Consts.DefaultChartHeight;
    private Series series;

    public void SetSeries(Series series)
    {
        this.series = new(series);
    }
    public void SetSize(int width, int height)
    {
        this.width = width;
        this.height = height;
    }
    public abstract Image GetChart();
}

А его конкретные реализации так:

public class ColumnChartBuilder : ChartBuilder
{
    private bool tornadoBar = false;
    public void SetTornadoMode()
    {
        tornadoBar = true;
    }
    Public override Image GetImage()
        => ChartUtils.CreateColumnChart(series,
                                            width,
                                            height,
                                            tornadoBar);
}

public class PieChartBuilder : ChartBuilder
{
    private bool showPercentages = false;
    public void SetShowPercentages()
    {
        showPercentages = true;
    }
    public override Image GetImage()
        => ChartUtils.CreatePieChart(series,
                                     width,
                                     height,
                                     showPercentages);
}

В представленном случае непосредственная генерация происходит в другом методе. Нам же проще. Если вдруг возникнет вопрос, зачем вообще тогда строитель, то напомню, что по условиям задачи число параметров может доходить до десятков :)

Ну и последнее, что нам осталось сделать, это ChartFactory:

public class ChartFactory()
{
    private ColumnChartBuilder CreateBaseColumnBuilder(
        Series series,
        bool tornado = false)
    {
        var builder = new ColumnChartBuilder();
        builder.SetSeries(series);
        if (tornado)
            builder.SetTornadoMode();

        return builder;
    }

    public Image CreateDefaultColumnChart(Series series,
                                          bool tornado = false)
        => CreateBaseColumnBuilder(series, tornado).GetChart();

    public Image CreateColumnChartWithSize(Series series,
                                           int width,
                                           int height,
                                           bool tornado = false)
    {
        var builder = CreateBaseColumnBuilder(series, tornado);
        builder.SetSize(width, height);
        return builder.GetChart();
    }

    private PieChartBuilder CreateBasePieBuilder(
        Series series,
        bool showPercentages = false)
    {
        var builder = new PieChartBuilder();
        builder.SetSeries(series);
        if (showPercentages)
            builder.SetShowPercentages();

        return builder;
    }

    public Image CreateDefaultPieChart(Series series,
                                       bool showPercentages = false)
        => CreateBasePieBuilder(series, showPercentages).GetChart();
    public Image CreatePieChartWithSize(Series series,
                                        int width,
                                        int height,
                                        bool showPercentages = false)
    {
        var builder = CreateBasePieBuilder(series, showPercentages);
        builder.SetSize(width, height);
        return builder.GetChart();
    }
}

Вот и всё! Теперь клиентскому коду достаточно просто позвать метод фабрики:

var pie = chartFactory.CreateDefaultPieChart(series);

И будет получена готовая картинка, содержащая график. Хотя здесь может возникнуть достаточно оправданный вопрос: а зачем вообще нужен распорядитель (в нашем случае — простая фабрика), если клиент может напрямую конструировать объект? Тут есть два соображения:

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

И на этом с теоретическим отступлением мы закончили. Можно возвращаться к теме.

Здесь же, правда, можно задать ещё один резонный вопрос: зачем я давал пример книжной реализации строителя на примере графиков, если мы хотели отрефакторить сам сбор статистики? Ответ простой: классическую реализацию показать хотелось, но в рефакторинге мы от неё отойдём. Можем себе позволить :)

Решение

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

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

Снова начнём с действующих лиц. Тут всё пока обыденно:

• Для итерации по датам реализуем DateIterator, от которого будут наследоваться YearIterator и MonthIterator для выполнения итераций по годам и месяцам соответственно.
• Продуктом сразу будут являться уже упоминавшиеся серии, на основе которых строятся графики.
• В качестве строителя определим StatisticsBuilder. Его методы будут добавлять серии и условия их учёта.
• Для работы с разными датасетами будем реализовывать разных строителей, но поскольку мы работаем с письмами, остановимся на одном — MailStatisticsBuilder.

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

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

Во-вторых, StatisticsBuilder добавляет учёт серий к самому себе и сам же заведует итератором. Выходит, в каком-то смысле строитель строит сам себя. Это может показаться странным, но на самом деле такой паттерн достаточно распространён. Все же знают про StringBuilder, не так ли? :)

Для прояснения ситуации нанесём сущности на диаграмму. В этот раз — с нотацией областей видимости:

Имея общее представление, нам должно быть просто это реализовать. Для начала приведём итераторы:

public abstract class StatisticsDateIterator
{
    public record struct DateEntry(int Year, int Month);

    public StatisticsDateIterator(DateTime firstIteration,
                                  DateTime lastIteration,
                                  int iterationCount)
    {
        LastIteration = FromDate(lastIteration);
        FirstIteration = FromDate(firstIteration);
        IterationsCount = iterationCount;
        CurrentDate = FirstIteration;
    }

    public int IterationsCount { get; }
    public int Iteration { get; private set; } = 0; 
    public DateEntry CurrentDate { get; protected set; }
    protected DateEntry FirstIteration { get; }
    protected DateEntry LastIteration { get; }

    public bool HasNext() => (Iteration <= IterationsCount);
    public DateEntry Next()
    {
        CurrentDate = GetNextDate(CurrentDate);
        Iteration++;
        return CurrentDate;
    }

    public abstract DateEntry GetNextDate(DateEntry date);
    public abstract DateEntry FromDate(DateTime date);
}

public class StatisticsYearIterator : StatisticsDateIterator
{
    // ....
    public override DateEntry GetNextDate(DateEntry date)
        =>  date with { Year = date.Year + 1 };
    public override DateEntry FromDate(DateTime date)
        => new(date.Year, 0);
}

public class StatisticsMonthIterator : StatisticsDateIterator
{
    // ....
    public override DateEntry GetNextDate(DateEntry date)
    {
        var month = date.Month + 1;
        var year = date.Year;
        if (month > 12)
        {
            month = 1;
            year++;
        }
        return new(year, month);
    }

    public override DateEntry FromDate(DateTime date)
        => new(date.Year, date.Month);
}

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

• Первые и последние даты итерации нужны для правильного их подсчёта, так как идём мы по датам.
• DateEntry нам понадобится, чтобы сгруппировать сущности по дате и иметь к ним быстрый доступ в Dictionary. DateTime в этом отношении не подойдёт, так как внутри он представлен как число с плавающей точкой, а напрямую их сравнивать нельзя.
• Номер итерации можно получить снаружи из-за того, что в старом алгоритме на него завязывались, и это упростит нам рефакторинг.

Здесь прояснили. Двигаемся к абстрактному StatisticsBuilder:

public abstract class StatisticsIterator
{    
    protected StatisticsDateIterator Iterator;
    protected StatisticsIterator(StatisticsIteratorType iteratorType,
                                 int rightYear,
                                 int rightMonth)
    {
        if (iteratorType == StatisticsIteratorType.Month)
            Iterator = new StatisticsMonthIterator(rightYear, rightMonth);
        else if (iteratorType == StatisticsIteratorType.Year)
            Iterator = new StatisticsYearIterator(rightYear, rightMonth);
        else
            throw new NotImplementedException();
    }
    public void Process()
    {
        for (var cur = Iterator.CurrentDate;
             Iterator.HasNext();
             cur = Iterator.Next())
        {
            ProcessInternal(cur);
        }

    }
    protected abstract void ProcessInternal(DateEntry date);

    protected Series GetSeriesWithZeroData(string name)
        => new(name, Enumerable.Repeat(0, Iterator.IterationsCount));
}

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

Всё начиналось с обработки писем, и мы, наконец, можем к ним вернуться. Вот как выглядит реализация абстрактного класса в MailStatisticsBuilder:

public class MailStatisticsBuilder : StatisticsBuilder
{
    private readonly Dictionary<DateEntry, List<Mail>> groupedMails;
    private readonly List<Action<List<Mail>, int>> _actions = new();

    public MailStatisticsBuilder(List<Mail> mails,
                        StatisticsIteratorType iteratorType,
                        int rightYear,
                        int rightMonth = 1)
        : base(iteratorType, rightYear, rightMonth)
    {
        groupedMails = mails.Select(x => (Iterator.FromDate(x.CreationTime), x))
                            .GroupBy(x => x.Item1, x => x.Item2)
                            .ToDictionary(x => x.Key, x => x.ToList());
    }

    protected override void ProcessInternal(DateEntry date)
    {
        if (groupedMails.TryGetValue(date, out var mails))
        {
            foreach (var action in _actions)
            {
                action.Invoke(mails, Iterator.Iteration);
            }
        }
    }

    // ....
}

Скорее всего, понятно ничего не стало, так как присутствует неясный _actions. Сейчас до этого дойдём — ответы спрятаны за четырьмя точками.

Но перед этим разберём обход данных. В конструкторе мы группируем письма по датам. Таким образом, когда в ProcessInternal мы будем переходить на следующую итерацию, у нас сразу будет доступ к письмам в этом промежутке времени. Также может возникнуть вопрос, почему не вынести всю логику с _actions в родительский класс. Однако реализация именно такая из-за того, что датасет может быть очень разнороден, поэтому большого выигрыша это не даст.

А теперь к _actions. Всё это время я тактично обходил стороной возвращаемое значение, которое было одним из камней преткновения в изначальной реализации. Более того, в приведённых фрагментах кода на это не было ни намёка — тот же Process строителя возвращает void. И правда, как нам одновременно сохранить гибкость опционального учёта данных и безопасность проверок времени компиляции (т. е. не прибегать к услугам каких-нибудь словарей)? Достигнуть этого нам позволят две фишки C#: анонимные функции и out-параметры. И то, что их нельзя использовать вместе, для нас совсем не помеха :)

После взгляда на код всё должно быть ясно:

public MailStatisticsBuilder AddRequestsCount(string type, 
                                              out Series count,
                                              out Series read,
                                              out Series answered)
{
    var _count = GetSeriesWithZeroData(type);
    var _read = GetSeriesWithZeroData("Read");
    var _answered = GetSeriesWithZeroData("Answered");

    _actions.Add((List<Mail> mails, int iteration) =>
    {
        foreach (var mail in mails)
        {
            _count.Data[iteration]++;
            if (mail.Read)
                _read.Data[iteration]++;
            if (mail.HasAnswer)
                _answered.Data[iteration]++;
        }
    });

    count = count;
    read = _read;
    answered = _answered;

    return this;
}

Что мы видим:

• Возвращаемое значение мы получаем в виде out-параметров. Заполняться они будут только после вызова Process.
• Логика учёта данных через анонимную функцию добавляется в коллекцию _actions. Элементы коллекции далее будут вызываться в ProcessInternal.
• Ограничение на out-параметры в анонимных функциях обходится при помощи переменных-посредников. Благодаря тому, что классы являются ссылочным типом данных, на функциональности это никак не отражается.

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

public MailStatisticsBuilder AddTrialsActivations(out Series activations)
{
    var _activations = GetSeriesWithZeroData("Activated");

    _actions.Add((List<Mail> mails, int iteration) =>
    {
        foreach (Mail mail in mails)
        {
            if (mail is Trial trial && trial.Activated)
                _activations.Data[iteration]++;
        }
    });

    activations = _activations;
    return this;
}

Вот так будет выглядеть применение такого строителя:

var builder = new MailStatisticsBuilder(
    allTrials,
    StatisticsIteratorType.Month,
    Year,
    month);
                          
builder.AddRequestsCount("Trials", out var count, 
                                   out var read,
                                   out var answered)
       .AddTrialsActivations(out var trialsActivations)
       .Process();

И эти серии сразу будут готовы для подстановки в график.

Такое решение нам даёт как гибкость (мы можем использовать только то, что нам надо), скорость (всё считается за один проход), уверенность в нахождении проблем во время компиляции, так и максимальную модульность. Такая система отлично расширяется как в сторону конкретного строителя (у нас в проекте цепочки вызовов строителя могут достигать десятка методов), так и в разные их типы (при помощи него мы собираем как уже описанную информацию о запросах, так и данные с нашего Task Tracker-а). Вот таков результат нашего рефакторинга.

Послесловие

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

К сожалению, статический анализ не подскажет вам архитектурный подход для решения проблемы, но укажет на возможные ошибки в уже написанном коде. Если вы пишете на С, C#, C++ или Java, то можете попробовать PVS-Studio по этой ссылке.

А чтобы следить за выходом новых статей про качество кода, можете подписаться на:

• твиттер PVS-Studio;
• наш ежемесячный дайджест статей;
• мой личный блог.