Code review: мысли и факты

Code review

• регулярный code review делают меньше 50% команд shepard/492/ppt/ppt18.ppt
• 96% дефектов выявляется при просмотре кода в одиночку (не на формальном митинге) http://dl.acm.org/citation.cfm?id=167070
• количество ревьюеров не должно быть большим http://blog.smartbear.com/code-review/who-should-review-my-code/
• одиночный ревьюер находит примерно 50% дефектов, два ревьюера - примерно 75% http://www.leshatton.org/Documents/checklists_in_inspections.pdf
• разработчик, который перепроверяет свой же код, также находит примерно 50% дефектов http://smartbear.com/resources/whitepapers/best-kept-secrets-of-peer-code-review/
• количество проблем, обнаруженных во втором code review, составляет примерно 50% от количества найденных в первом, команды из 3-4-5 ревьюеров практически не дают преимущества перед двумя ревьюерами http://dl.acm.org/citation.cfm?id=331521
• code review проводится не новичками, но для новичков
• эффективность ревью сильно зависит от уровня ревьюера и его знания просматриваемого кода http://dl.acm.org/citation.cfm?id=331521
• лучшие разработчики, тим-лиды и архитекторы могут и должны тратить время на code review
• соответствие кода стандартам кодирования/форматирования должно проверяться автоматически до ревью
• наибольшее время в ходе ревью уделяется проблемам сопровождаемости и читаемости кода: поиск ошибок в чужом коде намного сложнее, чем поиск ошибок в своём, сначала его надо прочитать и понять http://dl.acm.org/citation.cfm?id=1592371 http://www.st.ewi.tudelft.nl/~mbeller/publications/2014_beller_bacchelli_zaidman_juergens_modern_code_reviews_in_open-source_projects_which_problems_do_they_fix.pdf
• некоторые компании проводят двухэтапное ревью: первый этап - зачистка, поиск и исправление проблем форматирования/читаемости, второй этап - собственно глубокое ревью
• можно выделить наиболее критические участки кода или изменения и проводить их ревью более тщательно

Корректность кода http://www.drdobbs.com/architecture-and-design/longing-for-code-correctness/240005803

• Functional correctness: does the code do what it is supposed to do – the reviewer needs to know the problem area, requirements and usually something about this part of the code to be effective at finding functional correctness issues
• Coding errors: low-level coding mistakes like using <= instead of <, off-by-one errors, using the wrong variable (like mixing up lessee and lessor), copy and paste errors, leaving debugging code in by accident
• Design mistakes: errors of omission, incorrect assumptions, messing up architectural and design patterns like MVC, abuse of trust
• Safety and defensiveness: data validation, threading and concurrency (time of check/time of use mistakes, deadlocks and race conditions), error handling and exception handling and other corner cases http://swreflections.blogspot.com/2012/03/defensive-programming-being-just-enough.html
• Malicious code: back doors or trap doors, time bombs or logic bombs http://security.stackexchange.com/questions/3704/how-to-review-code-for-backdoors
• Security: properly enforcing security and privacy controls (authentication, access control, auditing, encryption)

Maintainability:

• Clarity: class and method and variable naming, comments, …
• Consistency: using common routines or language/library features instead of rolling your own, following established conventions and patterns
• Organization: poor structure, duplicate or unused/dead code
• Approach: areas where the reviewer can see a simpler or cleaner or more efficient implementation

High risk code:

• Network-facing APIs
• Plumbing (framework code, security libraries….)
• Critical business logic and workflows
• Command and control and root admin functions
• Safety-critical or performance-critical (especially real-time) sections
• Code that handles private or sensitive data
• Old code, code that is complex, code that has been worked on by a lot of different people, code that has had a lot of bugs in the past – error prone code

High risk changes:

• Code written by a developer who has just joined the team
• Big changes
• Large-scale refactoring (redesign disguised as refactoring)