Code Review

Transcript

1. Code Review Tools and Process @rantav
2. Formal Code Review Meetings • A sit down meeting • Everyone looks at the code • Need to get prepared in advance • People bring up issues that need to be fixed before you can go ahead with commit
3. Formal Code Review - The Pain • Needs preparation • Time consuming • Hard to concentrate • No time to dig in and be thorough • Difficult to schedule the right ppl, especially at decentralized companies or projects • Synergy - Yes • Cost effective - No • Fun - well...
4. Code Review in Apache 1. Every change has a jira issue 2. A patch is uploaded 3. A committer reviews and writes comments per the patch 4. When LGTM, the committer commits
5. Email Code Review
6. Offline Code Review Tools • Jupiter - Eclipse prehistorical plugin • Mondrian - Google • Rietveld - Free and very limited Mondrian • ReviewBoard - The leading open source • Crubicle - from Atlassian ($$) • Code Collaborator - the leading $$ from SmartBear • Gerrit - git goodness
7. Mondrian
8. Rietveld http://code.google.com/p/rietveld/
9. Code Collaborator http://smartbear.com/codecollab.php
10. reviewboard http://www.reviewboard.org/
11. Gerrit http://code.google.com/p/gerrit/
12. Motivation • Teach • Learn • Maintain coding conventions • Find defects early • Get more ppl familiar with the code (no job security)
13. Motivation - by the numbers Average defect detection rate: 25% for unit testing 35% for functional testing 45% for integration testing Design and code reviews 55% at design review 60% at code review
14. Motivation - by the numbers Average defect detection rate: 25% for unit testing 35% for functional testing 45% for integration testing Design and code reviews 55% at design review 60% at code review
15. When? • Before commit? • After commit? • During commit (?)
16. Who? • Manager • Tech lead • Architect • Peers ◦ Any peer from the team ◦ The one most likely to have valuable input
17. Checklist (?) • Does the code build correctly? • Does the code execute as expected? • Is all new code tested? Altered code tests up to date? • Is the code readable? • Do you understand the code you are reviewing? • Has the developer tested the code? • coding conventions? • Are all functions, methods and classes documented? • Are complex algorithms and code optimizations adequately commented? • Error Handling • Resource Leaks • Thread Safeness • Is the code free of unintended infinite loops? • Are function parameters explicitly verified in the code? • Pending/TODO • assertions • abnormal terminations • Database Transactions • Correct synchronization • no deadlocks/livelocks • Bug Fix Side Effects • All the occurrences of the bug are fixed
18. Different Techniques - recap • email • jira patch process a la apache • over the shoulder • formal review at a meeting room • pasetbin • web tools - reviewboard and such • GitHub - fork-review-pull process
19. Tips for Effective Code Review • Psychology - Let's face it, there's a lot of psychology in it, so play nice. • Be professional, nothing is personal • Strive to understand, not criticize. • Get prepared ◦ understand the technology in use ◦ know the coding conventions ◦ read related code (classes/method being used) • Read Slow • Short
20. What's Short? • No more than 60 minutes • No more than 200 LOC (C code)
21. Challenges (אל המל) • Ego • The Hassle
22. The Book
23. Related • Pair Programming • Code Reading (Doug Crockford) • Static Analysis tools
24. Extra slides
25. Eye tracking 01 void main(void){ 02 int i, num, isPrime = 0; 03 04 printf("Input Number:"); 05 scanf("%d", &num); 06 07 i = 2; 08 while(i < num){ 09 if(num%i == 0) 10 isPrime = 1; 11 i = i + 1; 12 } 13 14 if(isPrime == 1) 15 printf("%d is prime number.¥n", num); 16 else 17 printf("%d is NOT prime number.¥n", num); 18 }
26. Eye tracking (2)
27. Eye tracking (3) => Now there's a physiological reason to write short method. => People who spend more time on the first scan find more issues and find them faster
28. Defect density vs LOC http://smartbear.com/white-paper.php?content=docs/articles/Case-Study.html Conclusion: Don't review too much code at once (<200-400 LOC)
29. Defect density vs LOC/hour Conclusion: Take your time (<500 LOC/hour) http://smartbear.com/white-paper.php?content=docs/articles/Case-Study.html
30. Author preperation Conclusion: Author preparation results in more efficient reviews http://smartbear.com/white-paper.php?content=docs/articles/Case-Study.html