How do I review the code
Denys Yahofarov May 2019
❤
Slide 2
Slide 2 text
05.06
/me
• I’ve Wrote a lot of code in Ruby, Scala, and
JavaScript
• Seen some awesome and crazy things (in
so@ware engineering)
• Now working for solarisBank as an engineer
• My team builds [payment] cards transacHon
processing
Slide 3
Slide 3 text
05.06
self.talk
• My own experience
• Non-technical and completely subjec9ve
• Use at your own risk
• All stunts were done by me, but hey… I’m
s9ll alive
Slide 4
Slide 4 text
What kind of code?
• Different teams
• Different products
!"
#$
%&
'(
)*
Slide 5
Slide 5 text
What kind of code?
• Different teams
• Different products
• Different urgency
• … and everything is connected
!"
#$
%&
'(
)*
Slide 6
Slide 6 text
Why reviewing at all?
• For sharing the knowledge
• Learning and coaching
• Discussing solu5ons async, close to the
code
• Keeping the codebase healthy
• A regulatory requirement in
Slide 7
Slide 7 text
My principles
Slide 8
Slide 8 text
The code brings value in master,
deployed to production*
* Unless it’s an PoC
Slide 9
Slide 9 text
No software is perfect
Slide 10
Slide 10 text
Contributors deserve respect.
No ma3er who they are
Slide 11
Slide 11 text
Think twice before posting a comment
Slide 12
Slide 12 text
Time is precious. Going back and forth
with comments is painful
Slide 13
Slide 13 text
No matter how “bad” the code is, there
should be something good
Slide 14
Slide 14 text
Code in master brings value
• Acceptance criteria should be covered. In tests
• No questions regarding the syntax and style – it
should be automated
• Merge PR straight away if it’s good to go
• … and I can always open a PR into the original one.
Asking for feedback
• Every approval is a tradeoff between "no value" and
"added technical debt
Slide 15
Slide 15 text
No software is perfect
• Comment only about the most important
things
• Mark if the feedback s8ll lets merging the PR
• … and approve merge, but yet list things to
improve
• I don’t always refer to “Boy-scout” rule
• I leave comments even if PR is merged already
Slide 16
Slide 16 text
Respecting people
• My assump)ons:
• The engineer did the best thing he or she could
• And don’t make the same mistakes mul)ple )mes on purpose
• And I don’t have a full context. Nor looked into all possibili)es
ways of solving the problem
• This means “I can be wrong in my judgments”:
• Use benchmarks
• Provide metrics (code quality, test coverage)
• I ask ques)ons to get a full understanding of the context
Slide 17
Slide 17 text
BTW, who had one of?..
!,",⁉ or
$,% and &
As an only comment to your PR
Slide 18
Slide 18 text
Commenting and being clear
• ! is ❌ a #$ (yet)
• Use simple English
• And, yes, it’s OK to add Emoji too
• Provide sample code. Not rewriAng a big part,
however
• Leave a final comment in PR, summarize your
comments
Slide 19
Slide 19 text
Save &me on async and sync communica&ons
• When I comment, it’s now my responsibility too to
clear it out
• It may take just 10 minutes for two people to
resolve a long-going conversation
• … and add the recap into the PR
Slide 20
Slide 20 text
Be positive
• No ma&er, how “bad” it is, there should be
something good. Find it and appreciate it.