Code Reviews - Remove the Pain, Increase Code Quality and Spread Knowledge

Code reviews can be painful not only for the ones whose code is being reviewed, but for the code reviewer as well - they don't need to be. In this session I'll offer some tips and real world experiences about how you can improve your code review process or how to start doing code reviews if you are not doing them already. Code reviews are an important way to not only improve the quality of your code, but also a way to catch and minimize defects early. Code review is also an excellent way to spread knowledge, not just about the code itself, but about what other developers are building.

Transcript

1. Code Reviews Code Reviews REMOVE THE PAIN, INCREASE CODE QUALITY AND REMOVE THE PAIN, INCREASE CODE QUALITY AND SPREAD KNOWLEDGE SPREAD KNOWLEDGE Barrie Selack Liferay, Inc.
2. WHY ARE YOU HERE? WHY ARE YOU HERE?
3. WHY AM I HERE? WHY AM I HERE?
4. THE PSYCHOLOGY OF COMPUTER PROGRAMMING THE PSYCHOLOGY OF COMPUTER PROGRAMMING Gerald M. Weinberg Gerald M. Weinberg (1971) (1971)
5. EGOLESS PROGRAMMING EGOLESS PROGRAMMING
6. STRUCTURED WALKTHROUGH STRUCTURED WALKTHROUGH
7. THE 10 COMMANDMENTS OF EGOLESS THE 10 COMMANDMENTS OF EGOLESS PROGRAMMING PROGRAMMING
8. UNDERSTAND AND ACCEPT THAT YOU WILL MAKE UNDERSTAND AND ACCEPT THAT YOU WILL MAKE MISTAKES. MISTAKES. The point is to find them early, before they make it into production. The point is to find them early, before they make it into production. Fortunately, except for the few of us developing rocket guidance Fortunately, except for the few of us developing rocket guidance software at JPL, mistakes are rarely fatal in our industry. We can, and software at JPL, mistakes are rarely fatal in our industry. We can, and should, learn, laugh, and move on. should, learn, laugh, and move on.
9. YOU ARE NOT YOUR CODE. YOU ARE NOT YOUR CODE. Remember that the entire point of a review is to find problems, and Remember that the entire point of a review is to find problems, and problems will be found. Don’t take it personally when one is problems will be found. Don’t take it personally when one is uncovered. uncovered.
10. NO MATTER HOW MUCH “KARATE” YOU KNOW, NO MATTER HOW MUCH “KARATE” YOU KNOW, SOMEONE ELSE WILL ALWAYS KNOW MORE. SOMEONE ELSE WILL ALWAYS KNOW MORE. Such an individual can teach you some new moves if you ask. Seek Such an individual can teach you some new moves if you ask. Seek and accept input from others, especially when you think it’s not and accept input from others, especially when you think it’s not needed. needed.
11. DON’T REWRITE CODE WITHOUT CONSULTATION. DON’T REWRITE CODE WITHOUT CONSULTATION. There’s a fine line between “fixing code” and “rewriting code.” Know There’s a fine line between “fixing code” and “rewriting code.” Know the difference, and pursue stylistic changes within the framework of a the difference, and pursue stylistic changes within the framework of a code review, not as a lone enforcer. code review, not as a lone enforcer.
12. TREAT PEOPLE WHO KNOW LESS THAN YOU WITH TREAT PEOPLE WHO KNOW LESS THAN YOU WITH RESPECT, DEFERENCE, AND PATIENCE. RESPECT, DEFERENCE, AND PATIENCE. Non-technical people who deal with developers on a regular basis Non-technical people who deal with developers on a regular basis almost universally hold the opinion that we are prima donnas at best almost universally hold the opinion that we are prima donnas at best and crybabies at worst. Don’t reinforce this stereotype with anger and and crybabies at worst. Don’t reinforce this stereotype with anger and impatience. impatience.
13. THE ONLY CONSTANT IN THE WORLD IS CHANGE. THE ONLY CONSTANT IN THE WORLD IS CHANGE. Be open to it and accept it with a smile. Look at each change to your Be open to it and accept it with a smile. Look at each change to your requirements, platform, or tool as a new challenge, rather than some requirements, platform, or tool as a new challenge, rather than some serious inconvenience to be fought. serious inconvenience to be fought.
14. THE ONLY TRUE AUTHORITY STEMS FROM THE ONLY TRUE AUTHORITY STEMS FROM KNOWLEDGE, NOT FROM POSITION. KNOWLEDGE, NOT FROM POSITION. Knowledge engenders authority, and authority engenders respect – Knowledge engenders authority, and authority engenders respect – so if you want respect in an egoless environment, cultivate so if you want respect in an egoless environment, cultivate knowledge. knowledge.
15. FIGHT FOR WHAT YOU BELIEVE, BUT GRACEFULLY FIGHT FOR WHAT YOU BELIEVE, BUT GRACEFULLY ACCEPT DEFEAT. ACCEPT DEFEAT. Understand that sometimes your ideas will be overruled. Even if you Understand that sometimes your ideas will be overruled. Even if you are right, don’t take revenge or say “I told you so.” Never make your are right, don’t take revenge or say “I told you so.” Never make your dearly departed idea a martyr or rallying cry. dearly departed idea a martyr or rallying cry.
16. DON’T BE “THE CODER IN THE CORNER.” DON’T BE “THE CODER IN THE CORNER.” Don’t be the person in the dark office emerging only for soda. The Don’t be the person in the dark office emerging only for soda. The coder in the corner is out of sight, out of touch, and out of control. This coder in the corner is out of sight, out of touch, and out of control. This person has no voice in an open, collaborative environment. Get person has no voice in an open, collaborative environment. Get involved in conversations, and be a participant in your office involved in conversations, and be a participant in your office community. community.
17. CRITIQUE CODE INSTEAD OF PEOPLE – BE KIND TO CRITIQUE CODE INSTEAD OF PEOPLE – BE KIND TO THE CODER, NOT TO THE CODE. THE CODER, NOT TO THE CODE. As much as possible, make all of your comments positive and As much as possible, make all of your comments positive and oriented to improving the code. Relate comments to local standards, oriented to improving the code. Relate comments to local standards, program specs, increased performance, program specs, increased performance,
18. THE PSYCHOLOGY OF COMPUTER PROGRAMMING THE PSYCHOLOGY OF COMPUTER PROGRAMMING Gerald M. Weinberg Gerald M. Weinberg (1971) (1971)
19. YOU MUST HAVE A CODING STANDARDS YOU MUST HAVE A CODING STANDARDS DOCUMENT DOCUMENT
20. EVERYONE SHOULD HAVE THEIR CODE REVIEWED EVERYONE SHOULD HAVE THEIR CODE REVIEWED
21. KNOWING OTHERS WILL BE LOOKING AT YOUR KNOWING OTHERS WILL BE LOOKING AT YOUR CODE WILL MAKE YOU WRITE BETTER CODE. CODE WILL MAKE YOU WRITE BETTER CODE.
22. A CODE REVIEW IS NOT... A CODE REVIEW IS NOT... (or at least, should not be...) (or at least, should not be...)
23. NOT JUST BE SOMETHING YOU HAVE TO DO NOT JUST BE SOMETHING YOU HAVE TO DO Must be a priority ... from the top down
24. NOT BE SOMETHING TO DREAD NOT BE SOMETHING TO DREAD Enjoy Learn Socialize
25. NOT A CONTEST NOT A CONTEST
26. NOT A WITCH-HUNT NOT A WITCH-HUNT
27. NOT A WAY TO MAKE EVERYONE CODE LIKE YOU NOT A WAY TO MAKE EVERYONE CODE LIKE YOU
28. NOT BE A PLACE TO ENFORCE STANDARDS NOT BE A PLACE TO ENFORCE STANDARDS Before code review, run through a static analyzer FindBugs, Checkstyle FxCop, StyleCop
29. A CODE REVIEW SHOULD PROVIDE A CODE REVIEW SHOULD PROVIDE
30. EDUCATION EDUCATION Junior coders can learn from senior coders Senior coders can learn from junior coders
31. QUALITY IMPROVEMENT QUALITY IMPROVEMENT Easy to read Easy to follow
32. EARLY ISSUE DETECTION EARLY ISSUE DETECTION
33. ADVICE FOR THE REVIEWER ADVICE FOR THE REVIEWER
34. HAVE A CONVERSATION, DON'T ATTACK HAVE A CONVERSATION, DON'T ATTACK
35. KEEP TONE POSITIVE KEEP TONE POSITIVE
36. ASK FOR A (DESCRIPTION, THOUGHT PROCESS) ASK FOR A (DESCRIPTION, THOUGHT PROCESS)
37. DON'T FORGET TO PRAISE DON'T FORGET TO PRAISE
38. ADVICE FOR THE REVIEWEE ADVICE FOR THE REVIEWEE
39. CHECK YOUR CODE, BEFORE THE REVIEW CHECK YOUR CODE, BEFORE THE REVIEW
40. OFFER SUGGESTIONS TO ADD TO THE STANDARDS OFFER SUGGESTIONS TO ADD TO THE STANDARDS
41. REMEMBER ... IT'S ONLY CODE REMEMBER ... IT'S ONLY CODE
42. CODE REVIEW PROCESS CODE REVIEW PROCESS
43. PROVIDE CODE AT LEAST 24 HOURS BEFORE THE PROVIDE CODE AT LEAST 24 HOURS BEFORE THE REVIEW REVIEW The EOD before a morning review is not 24 hrs
44. CODE MUST HAVE LINE NUMBERS CODE MUST HAVE LINE NUMBERS
45. TIME LIMIT 1-2 HOURS TIME LIMIT 1-2 HOURS
46. REVIEW IN SMALL CHUNKS REVIEW IN SMALL CHUNKS
47. DON'T WAIT UNTIL THE END OF A PROJECT DON'T WAIT UNTIL THE END OF A PROJECT
48. ROTATE THE LEADER POSITION ROTATE THE LEADER POSITION Makes everyone feel they are equal Different viewpoints More focus on code by multiple people
49. CONSIDER A MODERATOR TO CONSIDER A MODERATOR TO Focus on code Stay on topic Keep the review moving
50. HOW ABOUT CODE REVIEWS WITH OFF-SHORE HOW ABOUT CODE REVIEWS WITH OFF-SHORE TEAMS? TEAMS?
51. HIGH IMPACT INSPECTION HIGH IMPACT INSPECTION Andrew Stellman http://www.stellman-greene.com
52. HIGH IMPACT INSPECTION HIGH IMPACT INSPECTION Moderator sends out code Everyone reviews code Notes sent to the moderator Moderator compiles list Moderator sends list to the team Review is now focused on the list
53. QUESTIONS QUESTIONS
54. THANK YOU THANK YOU You have been a wonderful audience!