Code Review Best Practice

We know that Code Reviews are a Good Thing. We probably have our own personal lists of things we look for in the code we review, while also fearing what others might say about our code. How to we ensure that code reviews are actually benefiting the team, and the application? How do we decide who does the reviews? What does “done” look like?

In this talk, Trisha will identify some best practices to follow. She’ll talk about what’s really important in a code review, and set out some guidelines to follow in order to maximise the value of the code review and minimise the pain.

Transcript

1. Code Review Best Practice Trisha Gee (@trisha_gee)
 Developer Advocate & Java Champion, JetBrains
3. This code works
4. Having Opinions On Code Is An Occupational Hazard
5. Having Opinions On Code Is An Occupational Requirement
6. Are we harder on other people’s code than our own?
7. What should we be looking for in a Code Review?
8. http://jb.gg/book/codereview
9. • Gateway Reviews • Knowledge Sharing • Early Design Feedback Different workflows https://blog.jetbrains.com/upsource/tag/code-review-workflows/
10. What should you look for when reviewing code?
11. It Depends
12. My First Code Review My job is to Find Problems
13. Anti-Pattern Nit picking
14. Anti-Pattern Design changes when the code works
15. Anti-Pattern Inconsistent feedback
16. Anti-Pattern The Ghost Reviewer
17. Anti-Pattern Ping pong reviews
18. Developers hate code reviews
19. Code Reviews are a 
 Massive Waste of Time
20. Take a step back…
21. 1. Why?
22. Ensure code meets standards
23. Find bugs
24. Ensure code does what it’s supposed to
25. Check code is understandable
26. Share knowledge
27. Collaborate on design
28. Evolve application code
29. 2. When?
30. • During implementation? • When it’s ready to merge? • After it’s been merged? When do you review?
31. • When everyone agrees? • When a gatekeeper agrees? • When all comments are addressed? When is the review complete?
32. 3. Who?
33. Who reviews the code?
34. Who signs it off?
35. 4. Where?
36. Pairing
37. Showing code to a colleague at a computer
38. Mob reviewing in a conference room
39. Remote screen-sharing
40. In the IDE, checking out a commit or branch
41. Using code review software
42. 5. What?
43. 1. Why 2. When 3. Who 4. Where Requires you to know:
44. • Fit with the overall architecture • SOLID principles, Domain Driven Design, Design Patterns or other paradigms of choice • New code follows team’s current practices • Code is in the right place • Code reuse • Over-engineering • Readable code and tests • Testing the right things • Exception error messages • Subtle bugs • Security • Regulatory requirements • Performance • Documentation and/or help files been updated • Spelling, punctuation & grammar on user messages What to look for https://blog.jetbrains.com/upsource/tag/what-to-look-for/
45. Human reviewers should be doing what cannot be automated
46. Understand the constraints
47. Why: Knowledge Sharing Purpose isn’t to reject the code
48. Why: Knowledge Sharing Focus is on learning what the code does and why
49. When: At the End Too late for design
50. When: At the End Should have set of checks
51. 6. How?
52. Automate Everything You Can
53. Submitting for review Reviews should be small
54. Submitting for review Annotate your code
55. Reviewing Should be clear Who is reviewing
56. Reviewing Respond in a timely fashion
57. Reviewing Checklist of What to look for
58. Comments Bear in mind Why, When and What
59. Comments Be constructive
60. Comments Be specific
61. Accept or Reject
62. Accept or Raise Concern Next steps should be clear
63. Making changes Respond in a timely fashion
64. Making changes Respond to comments
65. Resolving The goal is to accept the review
66. Resolving Should be clear Who signs it off
67. Resolving …and When
68. Code Reviews Suck Less When…
69. 1. Why 2. When 3. Who 4. Where 5. What 6. How The process is clear
70. Not to prove how clever you are The Goal Is To Ship The Code
71. http://bit.ly/CRGood