Code Review as a Collabora/ve
Journey
Hiroshi Kurokawa, Takafumi Nanao, mixi, Inc., DroidKaigi 2019
Slide 2
Slide 2 text
What is Code Review For?
Slide 3
Slide 3 text
What is Code Review NOT For?
• Finding out a programming bug
• Code forma3ng, lin6ng
♻
• Discussing the specifica6on
#
Slide 4
Slide 4 text
Code Review is for Collabora0on
• Sharing knowledge
!
• Sharing skill
"
• Making a good product through discussion
Slide 5
Slide 5 text
What Informa,on is required for Code
Review?
• Goal
• Background
• Strategy
• Changes made
• Verifica:on steps
Slide 6
Slide 6 text
No content
Slide 7
Slide 7 text
The Story of Code Review
Photo by Annie Spra0 on Unsplash
Slide 8
Slide 8 text
1. Understand the Background and
the Goal
Photo by Christopher Burns on Unsplash
Slide 9
Slide 9 text
2. Go through the Strategy and
What was Made
Photo by Annie Spra0 on Unsplash
Slide 10
Slide 10 text
3. Verify if the Change Really Takes
You to the Goal
Photo by delfi de la Rua on Unsplash
Slide 11
Slide 11 text
4. Look into Every Changes One by
One
Photo by Caleb Jones on Unsplash
Slide 12
Slide 12 text
5. Look Back on the Change from
the Goal
Photo by Vlad Bagacian on Unsplash
Slide 13
Slide 13 text
6. Have a Conversa-on with the
Reviewee
Photo by rawpixel on Unsplash
Slide 14
Slide 14 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
3. Verify if the change really takes you to the goal
4. Look into every changes one by one
5. Look back on the change from the goal
6. Have a conversaDon with the reviewee
Photo by Annie Spra0 on Unsplash
Slide 15
Slide 15 text
1. Understand the Background and the Goal
Slide 16
Slide 16 text
Understanding the
background
• Read the descrip.on
• Are enough details wri6en to review it
or is it using pull request template?
Slide 17
Slide 17 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
Slide 18
Slide 18 text
Strategy
• organize informa-on
• review the strategy
Photo by Jamie Street on Unsplash
Slide 19
Slide 19 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
3. Verify if the change really takes you to the goal
Slide 20
Slide 20 text
Opera&onal check
Checkout the branch and build it locally.
The git worktree command is very useful.
git worktree add ~/code-review/branch_name branch_name
android-studio.sh ~/code-review/branch_name
Slide 21
Slide 21 text
Build
• Warnings in build log or logcat
• Try proguard build or release build when
relevant file was changed
Photo by Randy Fath on Unsplash
Slide 22
Slide 22 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
3. Verify if the change really takes you to the goal
4. Look into every changes one by one
Slide 23
Slide 23 text
UI
• Is it along the guidelines of Material
Design or your team's?
• Is it not like iOS UI?
• Is it using anima
Slide 24
Slide 24 text
UI behind UI
• Is it handling errors?
• Does it work even with no data?
• add debug code directly
• Airplane mode
Photo by Mikhail Vasilyev on Unsplash
Slide 25
Slide 25 text
Lifecycle
• Does it work a-er returning from
another screen?
• Does it work a-er screen rota6on?
• Does it work a-er the process was
killed?
• Does it manage the Disposable or
Closable at the right 6ming
• Can it handle bu
Slide 26
Slide 26 text
Performance
• Is working fast? 1s is slow.
• Does it save ba7ery?
• Doesn't any crash happen?
Photo by Jonathan Chng on Unsplash
Slide 27
Slide 27 text
Read code
• use Android Studio
• If you have ques5on, don't hesitate to
check actual opera5on.
• Is there any newly unnecessary code by
that change.
Photo by Maximilian Weisbecker on Unsplash
Slide 28
Slide 28 text
Layout
• unnecessary stuffs
• unnecessary nest
• deprecated components
Photo by Jose Alejandro Cuffia on Unsplash
Slide 29
Slide 29 text
Code format
• share your coding rules
• forma3ng
• rules of lint
# .gitignore
!.idea/codeStyles
!.idea/inspectionProfiles
Photo by Fabio Santaniello Bruun on Unsplash
Slide 30
Slide 30 text
3rd party libraries
• Is it trustworthy?
• Is it maintained well?
• Are there be6er alterna7ves?
Photo by Glen Noble on Unsplash
Slide 31
Slide 31 text
Leaks
• Use it watching the profiler of AS
• Does it have a strong reference to
Ac8vity?
• Does it close Closable?
Photo by Luis Tosta on Unsplash
Slide 32
Slide 32 text
Tests
• Are proper tests added?
• Are tests enough for addi3onal
condi3ons or boundary values?
Photo by Nicolas Thomas on Unsplash
Slide 33
Slide 33 text
Side effects
• Adding Permission
• Moving Applica4on class
Photo by rawpixel on Unsplash
Slide 34
Slide 34 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
3. Verify if the change really takes you to the goal
4. Look into every changes one by one
5. Look back on the change from the goal
Slide 35
Slide 35 text
Look back
• Check if your opinion or recogni1on is
right.
• Imagine what you would do if you were
the reviewee.
Slide 36
Slide 36 text
1. Understand the Background and the Goal
2. Go through the strategy and what was made
3. Verify if the change really takes you to the goal
4. Look into every changes one by one
5. Look back on the change from the goal
6. Have a conversa+on with the reviewee
Slide 37
Slide 37 text
Values
We should share common values in our
team.
• Easy to read and change
• Proper package name
Photo by Lina Trochez on Unsplash
Slide 38
Slide 38 text
Give your feedback
• Feedback details to improve the code
• Use code sugges7on
• Tell condi7ons and steps to repro bug
• Compliments are welcome