Инспекция кода. Итоги



    Инспекция кода — это хорошо. Мы используем эту технику в своих проектах не так давно — около трех месяцев, — однако положительные результаты налицо. Мы уже рассказывали на Хабре о внедрении инспекций в процесс разработки, о документообороте при инспекциях, рассказывали о том, как можно оптимизировать процесс инспектирования с помощью инструмента Code Collaborator. Сегодня мы хотим подвести итоги и представить результаты, которых нам удалось достичь за время инспектирования. Поехали!..

    На самом деле, плюсов много, поэтому в этом топике мы коснемся лишь нескольких основных.

    Утвердился общий стиль написания кода. Конечно, и до внедрения инспектирования в нашей команде существовало некое представление о том, что такое хорошо и что такое плохо, которого все старались придерживаться. Однако благодаря инспекциям общий стиль был окончательно отточен и фактически «закрепился в голове» у каждого разработчика.

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

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

    Активизировался обмен опытом и знаниями. Теперь мы чаще используем в разработке успешные методики и практики коллег, реализованные в других частях системы. Подобный опыт, на наш взгляд, может передаваться только через чтение чужого кода: другого пути просто не существует. Любой метод нужно «пережить» самому, лишь тогда его может сделать «своим», освоить. Для такого «погружения» нет ничего лучше, чем хороший пример чужого кода из знакомой предметной области: это куда эффективнее абстрактных геометрических примеров типа shape, circle и box.

    Кодовая база стала более «сопровождаемой». Здесь целый список улучшений:

    • магические константы блокируются проверяющими;
    • уменьшилось дублирование кода (DRY);
    • алгоритмы стали проще (KISS);
    • документация классов и публичных методов улучшилась значительно (практически на каждый публичный класс и метод инспекторы требуют добавить документацию);

    Единственный недостаток внедрения инспекций кода — увеличение времени разработки. На начальном этапе внедрения время увеличилось на 50—100% (иногда бывало и больше), но в результате в общем репозитории стало гораздо меньше некачественного кода, меньше «шлака».

    В данный момент, после первоначальной отладки всех процессов, дополнительные временные затраты сократились до примерно 20—50%. Основной всплеск связан с притиркой команды к общему стилю кодирования; много времени ушло на принятие общих практик и методик. Для опытных команд, на наш взгляд, нормальное среднее увеличение времени разработки — 10%.

    Checklist


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

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

    Суммируя накопленный опыт инспектирования, мы начали понимать, как нужно составлять контрольный список. Вот как выглядит наш список на данный момент.

    Контрольный список — памятка инспектора Positive Technologies

    1. Корректность реализации предметной области.

    • Функциональность реализована в полном объеме.
    • Соответствие спецификациям.
    • Верность заданных констант.
    • Обоснованность принятых допущений и соглашений.
    • Верная последовательность обработки.

    2. «Читабельность» кода.
    Тут главный контрольный вопрос нужно задать самому себе: «Смогу ли я в случае необходимости добавить новую фичу или пофиксить баг в инспектируемом коде?». Ответ должен быть положительным, в противном случае автору следует изменить код.

    3. Наличие и полнота тестового покрытия.

    • Все публичные классы и методы тестируются.
    • Тестируются граничные условия.
    • Достаточность тестирования «положительных веток» выполнения.
    • Достаточность тестирования «отрицательных веток» выполнения.

    4. Стоит добавить в свою копилку хорошие практики из чужого кода.

    5. Корректность многопоточного кода.

    • Доступ к общим данным синхронизирован.
    • Отсутствуют deadlocks.
    • Для синхронизации используется идиома RAII.
    • Корректно обрабатывается возврат ресурсов в обработчиках ошибок.
    • Нет утечки ресурсов.

    6. Код обрабатывает возможные ошибки корректно.

    7. Отсутствие видимых нежелательных побочных эффектов в других частях системы.

    8. Корректность языковых конструкций.

    • Применяются верные идиомы используемого языка и фреймворка.
    • Использование корректных библиотек.
    • Отсутствие вновь изобретенных «велосипедов».
    • Отсутствие дублирования кода.

    9. Все переменные проинициализированы правильными значениями.

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

    Итак, почему же инспекции настолько эффективны? Часто бывает, что при проектировании или работе с кодом у разработчика в какой-то момент «замыливается глаз», ум накрепко привязывается к определенному проектному решению или конкретному фрагменту кода. Инспекция является простым способом обратиться к коллективному разуму, который обладает большей гибкостью и свободой, — по принципу «две головы лучше».

    Спасибо за внимание! Желаем вам приятных инспекций :)
    Positive Technologies
    115.11
    Company
    Share post

    Comments 27

      +2
      Спасибо за отчет.
      Вы не анализировали динамику багов, полученных от заказчика: возросло их количество или уменьшилось? На мой взгляд увеличение времени разработки должно компенсироваться уменьшением времени исправления багов.
        +1
        CR позволяет уменьшить количество багов, а не уменьшить время их исправления.
          0
          без CR:
            +1
            Извините, не туда нажал.
            Я думаю, что количество ошибок остается тем же, но время их обнаружения сокращается + сокращается время на понимание сути ошибки, так как вам о ней говорит технический специалист, а не заказчик. А это значит, что сокращается все таки время исправления ошибок, а не количество.
            Хотя если вы имеете ввиду только те ошибки, что дошли до заказчика, то соглашусь с вашей точкой зрения.
              +1
              Code review косвенно влияет на время устранения багов и уменьшает вероятность возниконовения багов (то есть их количество). Поясню. Код после review, как правило, более качественный, содержит логи в правильных местах, что ускоряет процесс выснения причины бага. Однако суммарное время потраченное на баг всеми участниками процесса (тестеры, разработчики, менеджеры) сокращается не очень значительно, потому что собственно поиск проблемы и ее устранение это относительно небольшая часть суммарно потраченного времени. С другой стороны, многие баги так и не возникнут, потому что код, прошедший review, содержит меньше велосипедов и потенциально опасных решений. То, что code review уменьшает вероятность возникноовения багов, значительно экономит время коллектива.
                0
                Согласен. Просто в статье указано, что для опытных команд время разработки с CR увеличивается на 10%. Я всегда верил в то, что в итоге в рамках проекта получаем все же экономию времени. Поэтому и задал вопрос.
                  0
                  Вот вы сами пишите время разработки, а не исправления багов. Тут, скорее, смысл может быть, например, в том что при разработке (ну, и при исправлении бага), можно вспомнить, что что-то подобное ты видел в одном review request: там была таже проблема, или нужная функция, только в другом модуле…
          +1
          Динамику багов не анализировали.
          Могу ответить на ваш вопрос только в рамках неточных моих личных субъективных оценок.
          По моему предыдущему проекту могу сказать, что дефектов при внедрении инспекций становится реально меньше, по моим ощущениям от 40% до 80%. Особенно инспекции эффективно снижают до 50% и ниже кол-во багов на старых проектах, в которых нет модульных тестов и код соответствующий. Ещё хочу отметить, что многопоточный код однозначно необходимо инспектировать, сложно одному всё предусмотреть, коллеги реально помогают находить потенциальные баги.

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

          Это уже профессиональная этика или даже гордость за свой труд, что я сделал хорошо и мне не стыдно это показать.
          И наоборот, самый плохой код, который я встречал и встречаю – код, который ни кто не смотрел, кроме самого разработчика.
          Даже по себе, сравнивая код, написанный приблизительно в одно и тоже время, один – ни кто кроме меня не смотрел, второй – мне проводили инспекцию. Так вот, догадайтесь, за какой код я испытываю стыд?  Отвечу – за первый, который кроме меня ни кто не смотрел. Лучше я три минуты буду выглядеть дураком, чем останусь им  Поэтому мой выбор – инспекция.

          Честно скажу по себе, когда у нас ещё не было инспекций кода и хотел отправить драйвер для нашей платы в mainline ядра linux, то код писал и проверял очень тщательно, потому что знал – разнесут в пух и прах за плохой код  Времени на него потратил на 20% больше, чем на обычный, зато код спокойно поняли и мои коллеги и инспекторы в ядре. +20% за написание качественного кода — это много или мало? Тут выбор каждый делает сам в зависимости от характера проекта.

          Имхо, конечно всё.
          +1
          Комикс ваш? Если нет, можно ссылку на оригинал указать?
            +1
              0
              Про сам комикс я знаю, интересует именно этот выпуск.
              +1
              Комикс сами смиксовали из разных выпусков комиксов explosm.net, так что ссылки на оригинал нет :)
                +1
                Чёт не надёжно у вас. Надо так:

                for(;;)
                 Self->Destroy();
                
                  +2
                  А можно у Вас картинку взять себе в одну презентацию? Обещаю указать подпись со ссылкой, какую скажете.
                    0
                    Да берите конечно. Не проблема! Не жалко :) Можно без надписей даже, ну или если хотите можно ссыль на сайт вставить www.ptsecurity.ru, но мы и так не обидимся.
                –4
                Ожидал, что в продолжении буду еще картинки. Ожидания не оправдались.
                  +1
                  Хочется поделиться своим опытом.

                  CR нужно только на этапе становления, организации, «взросления» — если хотите, команды. Когда есть сомнения в том, что «кто-то» может «фейлить». Тут необходимо объяснить человеку (в идеале — всей команде) почему это «плохо», и как это сделать правильно.

                  Когда команда «сформирована» (self-containing team), CR уже не имеет смысла, т.к. траты на него не стоят отдачи от следствия. Настает момент «доверия» и уверенности в каждом человеке в команде.

                  Бывают моменты, когда всеми правдами и неправдами, HR обеспечивает неимоверную «текучку» кадров в команде — в таких случаях этап «становления» практически бесконечен. Мне искренне жаль такие команды. В них, фактически, CR является единственным средством поддерживать код в legacy стиле.

                  Что-то еще хотел сказать, мысль улетела… может позже напишу…
                    +1
                    Code review не означает недоверия, мне кажется. Ревью применяется в серьезных компаниях без особой текучки кадров, вроде Google, просто потому что две головы всегда лучше чем одна.
                    А в проектах вроде софта для авионики вообще каждая строчка обосновывается документами в десятикратном размере.
                      +1
                      Нет, что вы — CR это никак не показатель недоверия! Это один из механизмов тестирования кода!

                      Наоборот, отсутствие необходимости CR характеризует «качество» команды. Понятное дело, все свести к минимуму нереально, но минимизировать затраты на «тестирование» (привет Automated testing) позволит рациональнее утилизировать свободные ресурсы.
                        +1
                        По мне так и то и другое хорошо — тесты для обеспечения преемственности (что новый код не ломает старый), ревью (преимущественно, конечно) — для обеспечения качества нового кода.
                      +1
                      Кроме градации «плохо-хорошо» есть «хорошо» и «качественно»
                      также бывают большие проекты, в которых «команда» не владеет кодом целиком (каждый участник может с ходу поправить любой участок), а над кодом работает много команд и т.п.

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

                        Ну, и, в своей команде я пропагандирую пассивное CR — есть свободная минутка, просмотри последние юзер-стори, глянь чекины. Есть замечания — сели, обсудили, поправили. Т.е., слава богу, команда на уровне автономной — качество кода уже не обсуждается. Качество продукта регулируется тестами — UT, AT, BVT.
                      0
                      Есть мнение, что самое лучшее Code Review — это парное программирование, ибо делается оно не ПОСЛЕ написания кода, а ВО ВРЕМЯ. И даже до.
                        +1
                        Лично сам я ощущаю так же, лучшая инспекция – парное программирование.
                        По-моему, не все проекты готовы к такой методике и не для всех она подходит.
                        Парное программирование плохо прижилось, например, при разработке встраиваемых систем. По-моему, потому что там большое кол-во задач, когда нужно читать документацию на какую-то микросхему или прибор и потом писать по документации код. Читают все с разной скоростью, усваивают так же с разной скоростью и в разных объёмах. Да и вообще, часто нужно с железкой что-то попробовать, поэкпериментировать. Тут второму просто делать нечего. Это лично моё субъективное мнение: почему при разработке встраиваемых систем парное программирование не практикуется или практикуется редко.

                        Хотя для части кода – лучше именно инспекция. Например, вы портируете драйвер сетевой карты, такой код лучше чтобы проинспектировали ещё и опытные коллеги. Они найдут – где я забыл засинхронизировать доступ к ресурсу или увидят возможность рекурсивного вызова того же обработчика прерывания, а я на такую ситуацию с напарником не заложился :)
                        По школе помню, что лучше «контролку» проверить, прежде чем её сдать, а то пара обеспечена . А ещё лучше, если её проверит мой сосед :) (мы кстати бывало так делали в школе, проверяли работы друг у друга – находили дефекты :)

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

                        Имхо, конечно всё.
                          0
                          Верно, ПП не должно полностью заменять code review.
                          В то же время неправильно полагать, что ПП не подходит, когда надо писать по документации код или много экспериментировать. Очень даже подходит. Вопрос привычки и дисциплины.
                        +1
                        Главное не доводить code review до придирок к ничего не значащим мелочам. Часто наблюдаю картину когда при code review большие и сложные для понимания блоки кода пролистываются, зато начинается хирургия вроде замены «1» + obj на string.Format(«1{0}», obj);

                        Нельзя допускать чтобы code review приводил к полной унификации всех решений, все-таки в программировании должен быть разумный допуск для творчества.
                          0
                          Self->Destory();

                          Вроде на Delphi похоже, но там не оператора ->
                          Впрочем этот код ВНЕЗАПНО есть в самом VCL в методе TObject.Free()

                          </занудство>

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