Upgrade to Pro — share decks privately, control downloads, hide ads and more …

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.

Trisha Gee

July 23, 2020
Tweet

More Decks by Trisha Gee

Other Decks in Technology

Transcript

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

    Developer Advocate & Java Champion, JetBrains

    View full-size slide

  2. This code works

    View full-size slide

  3. Having Opinions On Code Is An
    Occupational Hazard

    View full-size slide

  4. Having Opinions On Code Is An
    Occupational Requirement

    View full-size slide

  5. Are we harder on other people’s
    code than our own?

    View full-size slide

  6. What should we be looking for in a
    Code Review?

    View full-size slide

  7. http://jb.gg/book/codereview

    View full-size slide

  8. • Gateway Reviews
    • Knowledge Sharing
    • Early Design Feedback
    Different workflows
    https://blog.jetbrains.com/upsource/tag/code-review-workflows/

    View full-size slide

  9. What should you look for when
    reviewing code?

    View full-size slide

  10. My First Code Review
    My job is to Find Problems

    View full-size slide

  11. Anti-Pattern
    Nit picking

    View full-size slide

  12. Anti-Pattern
    Design changes when the code works

    View full-size slide

  13. Anti-Pattern
    Inconsistent feedback

    View full-size slide

  14. Anti-Pattern
    The Ghost Reviewer

    View full-size slide

  15. Anti-Pattern
    Ping pong reviews

    View full-size slide

  16. Developers hate code reviews

    View full-size slide

  17. Code Reviews are a 

    Massive Waste of Time

    View full-size slide

  18. Take a step back…

    View full-size slide

  19. Ensure code meets standards

    View full-size slide

  20. Ensure code does what it’s
    supposed to

    View full-size slide

  21. Check code is understandable

    View full-size slide

  22. Share knowledge

    View full-size slide

  23. Collaborate on design

    View full-size slide

  24. Evolve application code

    View full-size slide

  25. • During implementation?
    • When it’s ready to merge?
    • After it’s been merged?
    When do you review?

    View full-size slide

  26. • When everyone agrees?
    • When a gatekeeper agrees?
    • When all comments are addressed?
    When is the review complete?

    View full-size slide

  27. Who reviews the code?

    View full-size slide

  28. Who signs it off?

    View full-size slide

  29. Showing code to a colleague at a
    computer

    View full-size slide

  30. Mob reviewing in a conference
    room

    View full-size slide

  31. Remote screen-sharing

    View full-size slide

  32. In the IDE, checking out a commit
    or branch

    View full-size slide

  33. Using code review software

    View full-size slide

  34. 1. Why
    2. When
    3. Who
    4. Where
    Requires you to know:

    View full-size slide

  35. • 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/

    View full-size slide

  36. Human reviewers should be doing
    what cannot be automated

    View full-size slide

  37. Understand the constraints

    View full-size slide

  38. Why: Knowledge Sharing
    Purpose isn’t to reject the code

    View full-size slide

  39. Why: Knowledge Sharing
    Focus is on learning what the code
    does and why

    View full-size slide

  40. When: At the End
    Too late for design

    View full-size slide

  41. When: At the End
    Should have set of checks

    View full-size slide

  42. Automate Everything You Can

    View full-size slide

  43. Submitting for review
    Reviews should be small

    View full-size slide

  44. Submitting for review
    Annotate your code

    View full-size slide

  45. Reviewing
    Should be clear Who is reviewing

    View full-size slide

  46. Reviewing
    Respond in a timely fashion

    View full-size slide

  47. Reviewing
    Checklist of What to look for

    View full-size slide

  48. Comments
    Bear in mind Why, When and What

    View full-size slide

  49. Comments
    Be constructive

    View full-size slide

  50. Comments
    Be specific

    View full-size slide

  51. Accept or Reject

    View full-size slide

  52. Accept or Raise Concern
    Next steps should be clear

    View full-size slide

  53. Making changes
    Respond in a timely fashion

    View full-size slide

  54. Making changes
    Respond to comments

    View full-size slide

  55. Resolving
    The goal is to accept the review

    View full-size slide

  56. Resolving
    Should be clear Who signs it off

    View full-size slide

  57. Resolving
    …and When

    View full-size slide

  58. Code Reviews Suck Less
    When…

    View full-size slide

  59. 1. Why
    2. When
    3. Who
    4. Where
    5. What
    6. How
    The process is clear

    View full-size slide

  60. Not to prove how clever you are
    The Goal Is To Ship The Code

    View full-size slide

  61. http://bit.ly/CRGood

    View full-size slide