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

Code Review Best Practice

Trisha Gee
July 23, 2020
Technology
53
13k

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

See All by Trisha Gee
Are Your Tests Slowing You Down?
trishagee
0
340
Developer Productivity Engineering: What's in it for me? (LJC)
trishagee
1
180
Developer Productivity Engineering: What's in it for me? (DevBcn)
trishagee
0
900
Developer Productivity Engineering: What's in it for me?
trishagee
1
470
Career Advice for Programmers (2023)
trishagee
0
660
2015-2021
trishagee
2
17k
Staying Ahead of the Curve (2021)
trishagee
3
710
Life Beyond Java 8
trishagee
2
6k
Modern Java with IntelliJ IDEA
trishagee
4
810

Other Decks in Technology

See All in Technology
[SRE NEXT 2023 CfP]SREとして向き合うGenerative AI
hash_generate
0
190
GoでORMを自作してみる
harrisoneagle
0
950
Findyの開発生産性を上げるためにやったこと
puku0x
0
290
話を聴く技術 / listening skills
kaminashi
1
110
テスト駆動開発でダイエットに挑戦して失敗した話
terahide
0
510
20231011_LangChainコントリビュートのすゝめ
kzkmaeda
0
250
VR点検シュミレーションをA-Frameで作った話
kawajiritakayuki
0
1.5k
俺の推し活〜わたしとみしま〜XP祭り2023 LT
junki
1
520
AWSで作るGPSトラッキングシステム
miura55
0
190
Four Keysに基づくリリースプロセス改善とその成果 / Release process improvement based on the Four Keys and its results
ak1210
0
110
20230930_JAWS-FESTA_REJECT-CON_ControlTower
hiashisan
0
670
Component-Driven UI with ViewComponent Gem
rstankov
1
130

Featured

See All Featured
Understanding Cognitive Biases in Performance Measurement
bluesmoon
5
680
Dealing with People You Can't Stand - Big Design 2015
cassininazir
352
21k
GraphQLとの向き合い方2022年版
quramy
25
12k
Typedesign – Prime Four
hannesfritz
35
1.8k
The Success of Rails: Ensuring Growth for the Next 100 Years
eileencodes
26
5.5k
A Philosophy of Restraint
colly
195
15k
Designing with Data
zakiwarfel
93
4.5k
Designing the Hi-DPI Web
ddemaree
274
33k
JavaScript: Past, Present, and Future - NDC Porto 2020
reverentgeek
39
3.9k
Design by the Numbers
sachag
272
18k
RailsConf 2023
tenderlove
0
270
Creating an realtime collaboration tool: Agile Flush - .NET Oxford
marcduiker
10
1.2k

Transcript

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

    Developer Advocate & Java Champion, JetBrains

    View Slide

  2. View Slide

  3. This code works

    View Slide

  4. Having Opinions On Code Is An
    Occupational Hazard

    View Slide

  5. Having Opinions On Code Is An
    Occupational Requirement

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

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

    View Slide

  10. What should you look for when
    reviewing code?

    View Slide

  11. It Depends

    View Slide

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

    View Slide

  13. Anti-Pattern
    Nit picking

    View Slide

  14. Anti-Pattern
    Design changes when the code works

    View Slide

  15. Anti-Pattern
    Inconsistent feedback

    View Slide

  16. Anti-Pattern
    The Ghost Reviewer

    View Slide

  17. Anti-Pattern
    Ping pong reviews

    View Slide

  18. Developers hate code reviews

    View Slide

  19. Code Reviews are a 

    Massive Waste of Time

    View Slide

  20. Take a step back…

    View Slide

  21. 1. Why?

    View Slide

  22. Ensure code meets standards

    View Slide

  23. Find bugs

    View Slide

  24. Ensure code does what it’s
    supposed to

    View Slide

  25. Check code is understandable

    View Slide

  26. Share knowledge

    View Slide

  27. Collaborate on design

    View Slide

  28. Evolve application code

    View Slide

  29. 2. When?

    View Slide

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

    View Slide

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

    View Slide

  32. 3. Who?

    View Slide

  33. Who reviews the code?

    View Slide

  34. Who signs it off?

    View Slide

  35. 4. Where?

    View Slide

  36. Pairing

    View Slide

  37. Showing code to a colleague at a
    computer

    View Slide

  38. Mob reviewing in a conference
    room

    View Slide

  39. Remote screen-sharing

    View Slide

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

    View Slide

  41. Using code review software

    View Slide

  42. 5. What?

    View Slide

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

    View Slide

  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/

    View Slide

  45. Human reviewers should be doing
    what cannot be automated

    View Slide

  46. Understand the constraints

    View Slide

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

    View Slide

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

    View Slide

  49. When: At the End
    Too late for design

    View Slide

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

    View Slide

  51. 6. How?

    View Slide

  52. Automate Everything You Can

    View Slide

  53. Submitting for review
    Reviews should be small

    View Slide

  54. Submitting for review
    Annotate your code

    View Slide

  55. Reviewing
    Should be clear Who is reviewing

    View Slide

  56. Reviewing
    Respond in a timely fashion

    View Slide

  57. Reviewing
    Checklist of What to look for

    View Slide

  58. Comments
    Bear in mind Why, When and What

    View Slide

  59. Comments
    Be constructive

    View Slide

  60. Comments
    Be specific

    View Slide

  61. Accept or Reject

    View Slide

  62. Accept or Raise Concern
    Next steps should be clear

    View Slide

  63. Making changes
    Respond in a timely fashion

    View Slide

  64. Making changes
    Respond to comments

    View Slide

  65. Resolving
    The goal is to accept the review

    View Slide

  66. Resolving
    Should be clear Who signs it off

    View Slide

  67. Resolving
    …and When

    View Slide

  68. Code Reviews Suck Less
    When…

    View Slide

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

    View Slide

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

    View Slide

  71. http://bit.ly/CRGood

    View Slide