Роман Просин «Code Review — искусство развивать»

Говорят, повторенье мать ученья. Поверим на слово и рассмотрим тонкости искусства инспекции кода на примере языка C# так, как будто никогда о них не слышали.

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

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

Transcript

1. Code Review Искусство развивать
2. Меня зовут Просин Роман. Мой путь разработчика начался более 7 лет назад 2 из которых я работаю разработчиком в ICDB Team Райффайзенбанка. Всем привет 2 Почему Code Review? «Вы должны знать чему противостоите. Вы должны быть готовы!»
3. 3 Практика хорошего рецензирования Характеристики качества ПО или что посмотреть на Code Review
4. 4 Сложные сценарии. Автоинспекция кода. Главное отличие Статический анализ кода Метрики Критерии качества
5. 5 ROSLYN Sonarqube. Непрерывная инспекция кода Непрерывная инспекция кода с помощью SDK .NET Compiler Platform
6. Что такое Code Review? 6
7. В чём отличие? В чём отличие? 7
8. 8 Привет. Я Марк. Можете помочь? Нужно проревьювить мой код.
9. Код под увеличительным стеклом 9 [CakeMethodAlias] public static string Parameter( this ICakeContext context, string name, string prefix = "") => Parameter(context, name, prefix); [CakeMethodAlias] public static T Parameter<T>( this ICakeContext context, string name, T @default, string prefix = "") { … } StackOverflowException!
10. [CakeMethodAlias] public static T Parameter<T>( this ICakeContext context, string name, T @default, string prefix = "") { context.ThrowIfNull(nameof(context)); name.ThrowIfNullOrWhiteSpace(nameof(name)); return context.HasArgument(name) ? context.Argument(name, @default) : context.HasEnvironmentVariable(name) ? context.EnvironmentVariable($"{prefix}{name}", @default) : @default; } Код под увеличительным стеклом Высокая когнитивная сложность!
11. Что сказать Марку? [CakeMethodAlias] public static string Parameter( this ICakeContext context, string name, string prefix = "") => Parameter(context, name, prefix); Это работать не будет... Тут же будет StackOverflowException! Марк, почитай про бесконечную рекурсию. Если хочешь её применить, тогда нужно реализовать условие возврата, иначе ты получишь StackOverflowException! Ну ты и … Это же не возможно читать. Исправь это! Марк, в командных соглашениях есть пункт запрещающий объединять тернарные операторы в цепочки. Их сложно воспринимать. Замени на блоки, пожалуйста.
12. Плохой обзор кода x Оценка; x Критика в адрес автора; x Безапелляционное утверждение; x Не информативный комментарий;
14. 14 ✓ Лаконично; ✓ Дружелюбно; ✓ По существу; ✓ Подкрепил комментарий ссылками.
15. 15 Как принять feedback ✓Не обижаться; ✓Отвечать на каждый комментарий; ✓Благодарить;
16. Хвалите 16 ✓ Отличная работа! Молодец. ✓ О.. Отлично придумано. Теперь я могу достать параметр одной строкой. Красавчик! ✓ Классная фича. Спасибо!
17. Характеристики качества ПО
18. Что смотреть на Code Review 18 Цель Архитектура Абстракции Надежность Ремонтопригодность Безопасность
19. Ремонтопригодность 19 public void DoSomething( int param1, int param2, int param3, string param4, long param5) { ... } public void DoSomething(int param1, int param2, int param3, string param4) { ... }
20. Ремонтопригодность 20 var x = personList .OrderBy(person => person.Age) .OrderBy(person => person.Name) .ToList(); var x = personList .OrderBy(person => person.Age) .ThenBy(person => person.Name) .ToList(); • Коллекция вернётся отсортированной по имени • Не хватает тестов
21. int Pow(int num, int exponent) { num *= Pow(num, exponent - 1); return num; } int Pow(int num, int exponent) { if (exponent > 1) { num *= Pow(num, exponent - 1); } return num; } Надёжность StackOverflowException!
22. public class SqlInjection : Controller { // GET /SqlInjection/Authenticate public IActionResult Authenticate(string user) { var query = "SELECT * FROM Users WHERE Username = '" + user + "'"; var userExists = context.Users.FromSql(query).Any(); user = "' or 1=1 or ''='"; return Content(userExists ? "success" : "fail"); } } Безопасность
23. Безопасность public class SqlInjection : Controller { //… // GET /SqlInjection/Authenticate public IActionResult Authenticate(string user) { var query = "SELECT * FROM Users WHERE Username = {0}"; var userExists = context.Users.FromSql(query, user).Any(); return Content(userExists ? "success" : "fail"); } }
25. Какие могут быть сценарии Затягивание и блокировка или интеллектуальная усталость Поспешность в инспекции кода Внешние рецензенты Ложные дефекты. Траты времени на исправления того, что работает.
26. Затягивание и блокировка 0 1 2 3 4 5 6 7 8 0 20 40 60 80 100 Найдено дефектов Время (минут) Эффективность поиска 26
27. 0 20 40 60 80 100 120 140 160 0 200 400 600 800 1000 1200 1400 Плотность дефектов (KLOC) Скорость рецензирования (LOC/час) Зависимость плотности дефектов от скорости рецензирования Поспешность 27
28. Внешние рецензенты 28 85 48 0 10 20 30 40 50 60 70 80 90 Без самоанализа С самоанализом Средняя плотность дефектов (дефекты/KLOC) Влияние самоанализа на плотность дефектов
29. Ложные дефекты 29 95 96 98 97 95 92 94 93 99 91 92 97 94 5 4 2 3 5 8 6 7 1 9 8 3 6 86 88 90 92 94 96 98 100 102 Дефекты обнаруженные в фазе инспектирования В ходе самостоятельного анализа В рамках встреч
30. Возможные сценарии Затягивание и блокировка Поспешность Внешние рецензенты Ложные дефекты
31. Решения Не более 60 мин. 0 – 400 LOC/час Демонстрация Самоанализ
33. Главное отличие Объективность
34. Editorconfig • Стили наименования • Рекомендации по написанию кода .NET • Рекомендации по написанию кода C# • Правила форматирования C# • Стили именования
35. Реализуем CI конвейер
36. 36 Пишем Cake скрипты. Cake - это кроссплатформенная система автоматизации сборки с C# DSL Запуск Cake Scripts на CI сервере Cake запускает NUnit Console Runner и тесты Cake запускает Nuget и упаковывает артефакты в пакеты. Cake отправляет артефакты куда скажете Управляем версиями через GIT
37. Простой скрипт Task("Build") .Does(() => { var solution = GetFiles("../**/Cake.Addins.sln").FirstOrDefault(); if(solution is null) { throw new CakeException("Solution not found"); } MSBuild(solution); }); RunTarget(target); string target = Argument(nameof(target), "Analysis");
38. Переменная X не инициализирована int x; int y = x + 2; Статический анализ кода
39. Подключаем sonarqube
41. Настройка #addin nuget:?package=Cake.Sonar&version=1.1.18 #tool nuget:?package=MSBuild.SonarQube.Runner.Tool Setup(ctx => { Information("Running setup..."); sonarSettings = new SonarBeginSettings() { Url = "https://sonarcloud.io", Login = "f6509d3b36c7d016beace4046256488d844e9bec", Key = "code.review", Name = "Code Review", Version = "1.0.0.0", UseCoreClr = true, }; }); 41
42. Добавление задачи 42 Task("InitializeSonar") .Does(() => { SonarBegin(sonarSettings); }); Task("Analysis") .IsDependentOn("InitializeSonar") .IsDependentOn("Build") .Does(() => { SonarEnd(sonarSettings.GetEndSettings()); });
48. ☺ Не нужно явно запускать анализаторы ☺ Не нужно ждать сборки ☺ Анализаторы на основе Roslyn выполняются асинхронно, во время вашей работы. ☺ Объективность ☺ Не вызывает сомнений Преимущества Roslyn анализаторов 48
49. Правило устанавливаете вы 49 !
50. Полезное 50 ▪ Орам Э. Идеальная разработка ПО. Рецепты лучших программистов. / Орам Э., Уилсон Г. - СПб.: Питер 2012.— 592 с. ▪ https://cakebuild.net/ ▪ https://sonarcloud.io/
51. Спасибо за внимание! prosin-roma@ya.ru raiffeisen.ru