Code Review
Why & How
Ran Tavory, Ido Barkan
AppsFlyer Aug 2018
Slide 2
Slide 2 text
Code Review
Why?
Slide 3
Slide 3 text
● For our Customers
● For Us
Slide 4
Slide 4 text
For our
customers
The immune system
“Running Continuous Deployment
without an Immune System is
playing Russian Roulette”
Slide 5
Slide 5 text
The Immune
System
What is the Immune System?
● The line of defence will always
be broken
=> Solution: Multiple lines of
defence
Slide 6
Slide 6 text
The Immune System
Slide 7
Slide 7 text
The Immune
System
Code Review is part of the immune
system
● Coding
○ Code Review
○ ...
● Monitoring
○ ...
● Instrumentation
○ ...
● KPI Monitoring
○ ...
Slide 8
Slide 8 text
Code review - For us
● Improve our skill
○ Best way to Teach
○ Best way to Learn
● Maintain Coding conventions
● Find Defects early
● More devs familiar with the code
Slide 9
Slide 9 text
Programs
must be
written for
people to read
- Harold Abelson
But if we don’t test code readability,
how do we know it is readable?
Slide 10
Slide 10 text
Quality v/s
Speed ?
NO!
Quality is a requirement in order to
maintain speed
WHY?
Slide 11
Slide 11 text
Exponential Value (and complexity)
Slide 12
Slide 12 text
Code Review
How?
Slide 13
Slide 13 text
Committer
The committer’s flow
1. Create a branch
2. Push to GitLab
3. Create Merge Request
4. Assign to approver (single one)
5. … back and forth
6. Approval
7. Merge
Slide 14
Slide 14 text
Reviewer
The reviewer flow
1. Be informed
2. Review Code
3. Leave Comments
4. … back and forth
5. Approve when Happy
Slide 15
Slide 15 text
Async Code
Review
Async is preferred
● Code review is Async by default
○ The true code reading
○ Remote devs
● Move to Sync when not
converging
○ Only in rare cases
○ Document your discussion
Slide 16
Slide 16 text
Code Review Flow (Commiter)
Slide 17
Slide 17 text
Code Review Flow (Commiter)
Slide 18
Slide 18 text
Code Review Flow (Reviewer)
Slide 19
Slide 19 text
GitLab - Default Project Settings
Slide 20
Slide 20 text
Code Review
Best Practices
Slide 21
Slide 21 text
No content
Slide 22
Slide 22 text
What to look for in code review
● Keep away from checklists…
But…
● What I usually look for:
○ I understand
○ It makes sense
○ Aesthetics (naming, style etc)
○ SW Engineering (modularity, coherence, DRY etc)
○ Docs
○ Tests
Slide 23
Slide 23 text
Tips for successful review
● Play nice. Don’t hurt egos
● Be professional. It isn’t personal
○ Concentrate on the code, not the personality
● Understand, not criticize
● Get prepared
○ Technology
○ Product
○ Coding conventions
○ Related code
● Read slow
● Short code reviews
Slide 24
Slide 24 text
How short? What size is ideal?
Slide 25
Slide 25 text
What size is ideal?
● 100 - 300 LoC (research on C code, for Clojure - shorter)
● 30-60 minutes
Not Covered Today
● Linters
● Tests and Coverage
● CI with CR
● Design Review
● Readability and Ownership
● CR Checklists (?)
Slide 28
Slide 28 text
Thank you
Slide 29
Slide 29 text
References
● This all started here
https://community.appsflyer.com/t/code-review-practices/94
● https://smartbear.com/SmartBear/media/pdfs/best-kept-secrets-of-peer-code-r
eview.pdf