Code Review Best Practice

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.

3d6ace9554821d552146413bcdf874f6?s=128

Trisha Gee

July 23, 2020
Tweet

Transcript

  1. Code Review Best Practice Trisha Gee (@trisha_gee)
 Developer Advocate &

    Java Champion, JetBrains
  2. None
  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