Рефакторил-рефакторил, и почти всё переписал в итоге / Хабр

Дали мне недавно задачу написать тесты для одной CLI-тулзы. Это мне уже привычно и понимание, зачем тулза нужна, есть. Я только не знал, что меня ждёт в коде. Программист, писавший её, сделал гигантскую работу — претензий нет (не обижайся, пожалуйста, если читаешь это, но это стоит отдельной статьи). Там суммарно, наверно, порядка 30к строк кода написано. Нюанс в том, что, видимо, он раньше не писал на C#,

Так что тут я соберу для вас примеры, как нельзя писать и как стоит. Они подойдут не только для C#-программистов.

1. Аргументы

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

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

// Было (примерно):
static void Main(string[] args)
{
    if (args[0] == "a")
        Mathod1(args.Skip(1));
    ...
}

// Стало:
static void Main(string[] args)
{
    var config = ConfigReader.Read(args);
    // забываем о существовании args
    ...
}

2. Функциональное программирование

Сервис написан на C#, где всё — классы. И вполне логично было бы использовать этот подход, но оказалось, что абсолютно все методы — статические. Все данные передаются через параметры, которых могло быть до 15 в одном методе (к счастью, именно таких было не много).

Я понял, что мне придётся буквально всё переписать, используя объектно-ориентированный подход. Почему это так важно? Напомню, что моя главная задача — это написать тесты для этого сервиса. Если интеграционные ещё возможны, то юнит-тесты написать для такого кода невозможно. Для тестов вся статика, которая обращается к внешним данным, должна переехать куда-нибудь, что реализует соответствующий интерфейс (а это обращения к файлам, БД, консоли). Для этого, конечно, пришлось подключить DI.

// Было:
public static void Method(string p1, int p2, ..., bool p7)
{
    if (Console.ReadLine() == "yes")
        AnotherClass.AnotherMethod(p1, p3, p5, p7);
    ...
}

// Стало:
static void Main(string[] args)  // main всегда статический
{
    var builder = Host.CreateApplicationBuilder();
    ...    // указываем зависимости тут
    var app = builder.Build();
    ...
    await using var scope = app.Services.CreateAsyncScope();
    
    if (config.RunService)
    {
        var service = scope.ServiceProvider.GetRequiredService<IService>();
        await service.Run();
    }
  
    ...
}

internal class Service : IService
{
    private readonly IAnotherService anotherService;

    public Service(IAnotherService anotherService) =>
        this.anotherService = anotherService;

    public void SomeMethod(string p1, int p2, bool p3)  // сократил число параметров, где возможно
    {
        if (consoleService.Prompt() is "yes" or "y")
            anotherClass.AnotherMethod(p1, p2);
        ...
    }
}

3. Логи

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

Подключение DI открыло доступ к ILogger, чем я с удовольствием воспользовался. И во всём новом коде я переписывал вызовы статического метода на вызов логгера, не переписав руками и не скопировав ни одного сообщения (спасибо Copilot). Файлы подключил отдельно через хорошо знакомый (работал над ним в ещё на заре создания) Vostok от "Контура". Заработало всё сразу, быстро и красиво (позже дописал свой форматтер для логов в консоль — и стало ещё красивее и удобнее читать).

// Было:
Program.Log($"Error message in {something}: {exception.message}", isError: true);

// Стало:
logger.LogError(exception, "Error message in {Something}", something);

4. Универсальные классы

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

Мне предстояло метод за методом разложить всё это по новым нескольким десяткам классов (50 сервисов, 3 провайдера, 16 репозиториев) от 50 строк на класс. Естественно, все в DI, реализующие каждый свой интерфейс. Всё это заняло около 20 рабочих дней. В итоге в проекте стало порядка пары сотен файлов, но они понятно названы, разложены, их удобно читать и понимать. Это позволило после окончания всего процесса рефакторинга составить потом схему работы сервиса, указав зависимости сервисов друг от друга. Только после этого весь код сразу же обрёл смысл и пришло понимание, как оно там всё работает.

Старый код при создании классов я вообще не трогал. Только дописывал новый. Это исключит путаницу и позволит просто удалить весь старый код в конце.

Было: 20 файлов до 3000+ строк, отсутствие структуры и схемы, невозможность понять стороннему разработчику (несмотря на наличие readme для пользователя).
Стало: 200+ файлов, где единицы размером по 300+ строк, всё разложено, нарисована схема, легко понять и прочитать

5. Репозитории

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

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

// Было:
public static void Method(...)
{
    ...  // тут бизнес-логика

    using (var command = new SqlCommand(query, connection))
    {
        command.Execute();
    }

    ...  // снова бизнес-логика
}

// Стало:
public async Task Method(...)
{
    ...  // тут бизнес-логика
      
    var result = await someRepository.GetSomeData(p1, p2);
  
    ...  // снова бизнес-логика
}

6. Асинхронность

Она была, но только для того, чтобы оптимизировать выполнение в некоторых местах. В остальном абсолютно всё, даже обращения к БД, были синхронными. В консольном приложении это довольно важно, ведь во время обращения к БД окно просто замирает и ждёт выполнения. Конечно, одно обращение на фоне остального выполнения, которое длится очень долго, не так важно, но и всё остальное выполнение могло быть быстрее.

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

// Было:
public static void Method(...)
{
    ...
    SomeStaticClass.Method(...);
    ...
}

// Стало:
public async Task Method(...)
{
    ...
    await someClass.Method(...);
    ...
}

7. Исключения

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

Лично я предпочитаю использовать try-catch и throw только там, где это действительно уместно. Например, при обращении к БД на случай потери подключения или ошибки в данных. Перехватили исключение и передали сообщение. В остальном достаточно одного try-catch на самом верхнем уровне. И если программа выбросит исключение там, где мы его не ожидаем, то 99%, что где-то ошибка в логике (например, пропущена проверка на null reference). Поэтому почти все методы у меня стали возвращать string? с возможным текстом ошибки. Но там, где это логично. Если же метод уже что-то возвращал, я использовал именованный кортеж. Ранее я писал про своё отношение к throw и try-catch.

// Было:
public static int Method(...)
{
    try
    {
        ...
        if (wrongParameter)
            throw new Exception("Wrong parameter");
        ...
        return count;
    }
    catch (Exception ex)
    {
        Program.Log($"Error: {ex.Message}");
        throw;
    }
}

// Стало:
public async Task<(int? result, string? error)> Method(...)
{
    ...
    if (wrongParameter)
    {
        logger.LogError("Wrong parameter");
        return (null, "Wrong parameter");
    }
    ...
    return count;
}

8. Предупреждения

Выше я упомянул наличие NRE (Null Reference Exception). Такое бывает, когда не обращаешь внимание на предупреждения, которые IDE находит в проекте и в идеале показывает пользователю после компиляции. Там их было сотни, самого разного рода: от стилистических до потенциально опасных.

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

// Было:
SomeClassWithAVeryLongName? result = GetResult(...);
var x = result.Value / 2;

// Стало:
var result = GetResult(...);
var x = result is null ? 0 : result.Value / 2  // или (result?.Value ?? 0) / 2

9. Провайдеры

Помимо БД есть ещё другие источники данных и у нас в тулзе их ещё два типа: внешний и файлы. Как и везде, это было в статике. Естественно, напрямую использовался класс File.

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

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

// Было:
public static void Method(string fileName)
{
    if (File.Exists(fileName))
    {
        ...  // читаем данные из файла
    }
    
    ...
}

// Стало:
public async Task<string?> Method(string fileName)
{
    var (result, error) = await fileProvider.ReadData(fileName);
    if (error is not null) return error;

    ...
}

10. Структура файлов

Продолжим говорить про файлы. В них лежит JSON, но не простой, а в некоторых случаях совсем без структуры. Т.е. вместо того, чтобы прочитать его в нормальный класс или массив классов, мы читаем его во что-то другое: Dictionary<string, object?> (не самый плохой вариант при гибкой структуре), JsonElement (так себе, но тоже сойдёт), dynamic (хуже не придумаешь). Отгадайте, что я нашёл.

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

// Было:
var data = JsonSerializer.Deserialize<dynamic>(json);
var x1 = data[0].number;

// Стало:
var data = JsonSerializer.Deserialize<List<Dictionary<string, object?>>>(json);
var x1 = data[0].GetIntOrDefault("number") ?? 0;  // тут для этих случаев был написан метод расширения

11. Методы в классах

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

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

// Было:
public class Config
{
    public string ConnectionString { get; set; }

    public SqlConnection Connect() { ... }
}

// Стало:
internal class Config
{
    public string ConnectionString { get; set; } = null!;
}

internal class ConnectionProvider : IConnectionProvider
{
    public async Task<SqlConnection> Connect(config.ConnectionString) { ... }
}

12. Помощники и расширения

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

Чем же отличаются помощники (Helpers) от расширений (Extensions)? Как я выше написал, не стоит запихивать всё подряд в методы классов. В том примере метод уехал в сервис, но он подойдёт для не статического метода, когда у нас есть дополнительные зависимости. Но если лишних зависимостей нет, все данные имеются в нужном нам типе (классе или любом другом) или чуток снаружи, то в этом случае стоит использовать расширения.

Но есть отдельные случаи, когда стоит расширение превратить в помощника. Это, прежде всего, действия над простыми типами или одноразовые действия над часто используемыми типами. Т.е. "string data".ToIPArray() — очень плохая идея, особенно если это не может встретиться где угодно в коде. Но IDE теперь вам будет подсовывать этот метод к каждому строковому значению. Также SomeClass.OneTimeExtensionMethod() — тоже плохо, если используется только в одном месте — IDE также теперь будет его подсовывать к этому классу. В этих случаях надо писать помощников.

// Было:
var dto = data.ToDto();  // метод внутри класса, данных, выполняющий чужую работу (маппера)
var dtFormatted = dateTime.ToClientFormat();  // метод расширения на популярный тип

// Стало:
var dto = data.ToDto();  // метод-расширение (внутри нет зависимостей и маппер нужен в разных местах)
var dtFormatted = DateTimeHelper.ToClientFormat(dateTime);

13. Константы

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

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

// Было:
dict["type"] = "Money";

//Стало:
dict[Consts.Field.Type] = Consts.Type.Money;

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

14. Запросы к БД. Поля

Любые операции ввода-вывода — слабые места большинства программ, а особенно если это обращение к удалённой БД. Поэтому стоит уделить особое внимание на количество запросов и какие данные они возвращают. В тулзе мне встретились, наверно, большинство проблем.

Начнём с первой: разные запросы к одной и той же таблице за разными данными. Мы сначала получали список уникальных значений из таблицы, а потом шли по этому списку и каждый раз обращались к той же таблице за другими полями. Это было слишком. Но этого было не достаточно и внутри цикла мы обращались к за ещё одними данными в другую таблицу. В примере ниже на 10 элементов имеем теперь всего 2 запроса вместо 21.

// Было:
var sql = "SELECT id FROM table";
var ids = ...;  // получаем данные в виде списка
foreach (var id in ids)
{
    var dataSql = "SELECT name, value FROM table WHERE id = @id";
    var data = ...;  // получаем данные
    ...
    var moreDataSql = "SELECT id, name FROM table_2 WHERE x_id = @id";
    var moreData = ...;  // получаем данные
    ...
}

// Стало:
var sql = "SELECT id, name, value FROM table";
var data = ...;  // получаем данные в виде списка

var moreDataSql = "SEELCT id, name FROM table_2 WHERE x_id in @ids";
var moreData = ...;  // получаем данные в виде словаря, где [id] == item

foreach (var item in data)
{
    var md = moreData[item.id];
    ...
}

15. Запросы к БД. Повторы

Я нашёл очень часто повторяющиеся два запроса, причём данные в них тоже часто повторялись. "Это прекрасная причина для оптимизации," — подумал Штирлиц.

Если мы какие-то данные уже прочитали и они не должны меняться вообще или какое-то время, то можно или даже нужно закэшировать подобные запросы. В моём случае эти данные вообще никогда не менялись, что сильно упрощало задачу. Я дописал работу с кэшем. При наличии DI в проекте подключить его было делом двух строчек (подключение библиотеки и builder.Services.AddMemoryCache();). Кода стало чуть больше, но зато выигрыш в сотни раз за одно частое обращение.

// Было:
public static string GetValue(,,,)
{
    ...  // обращение к БД
    return result;
}

// Стало:
public async Task<string> GetValue(...)
{
    var key = ...;  // создаём ключ
    if (memoryCache.TryGetValue(key, out string? cached))
        return cached;

    ...  // обращение к БД

    memoryCache.Set(key, result, TimeSpan.FromMinutes(10));
    return result;
}

16. Запросы к БД. Существование

Встретился и страшный запрос на проверку существование через COUNT(*). Конечно, так можно проверять, но какой смысл в чтении всей таблицы, в которой могут быть миллионы записей, если нам достаточно одной строки, чтобы узнать, что запись уже существует?

Конечно, я не обошёл стороной все подобные запросы и поменял COUNT(*) на EXISTS.

// Было:
var existsQuery = "SELECT COUNT(*) FROM table WHERE name = @name";
var count = ...;  // выполняем запрос
return count > 0;

// Стало:
var existsQuery = @"
    SELECT CASE WHEN EXISTS (
        SELECT 1 FROM table WHERE name = @name
    ) THEN 1 ELSE 0 END";
var result = ...;  // выполняем запрос
return result > 0;  // или result == 1;

17. Конкатенация строк

Эта штука довольно очевидная, но я нередко её встречал в разных проектах и тут в том числе. Это неэффективная работа со строками, когда из множества кусочков надо собрать одну строку. Есть два подхода: List + string.Join() и StringBuilder. Я предпочитаю второй. Это, конечно, не касается рефакторинга, но оставлять такое в коде нельзя.

// Было:
var str = "values:";
foreach (var item in items)
    str += $" {item}";
return str;

// Стало:
var sb = new StringBuilder("values:");
foreach (var item in items)
    sb.Append($" {item}");
return sb.ToString();

18. Зависимости

Казалось бы, всё сделано. Что ещё? А случилось то, что не вызывает проблем в функциональном подходе: циклические зависимости. Их оказалось довольно много. При запуске выяснилось, что некоторые участки кода зависят от тех, которые парой уровней выше вызывают то, что вызывает текущий класс. В общем, это создало дополнительные трудности, которые не исправить изменением логики. Точнее это потребует слишком больших усилий. Поэтому я пошёл по простому пути и просто удалил проблемные зависимости, заменив их добавлением IServiceProvider с последующим созданием проблемной зависимости в виде отдельной переменной.

var service = serviceProvider.GetRequiredService<IService>();
await service.Method(...);

Тестирование

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

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

Приятного вам кода! И помните, что чистый код — это не только про названия и число строк в методе. Это гораздо больше и приходит не только с прочтением соответствующей литературы, но и с опытом разработки и чтения чужого кода (code review вам в помощь).

(Интересно, а ссылку на свой LinkedIn можно добавлять? Или только на Телеграм?)