Code Review Best Practices

For AppsFlyer internal teams

Transcript

1. Code Review Best Practices Ran Tavory, Appsflyer Aug 2018
2. The first rule of code review The first rule of code reviews is: • There are no rules in code reviews.
3. Code Review Objectives • Verify that the code is a correct and effective solution for the requirements at hand • Ensure that your code is maintainable • Increase shared knowledge of the codebase • Sharpen your team’s skills through regular feedback
4. The Flow
5. GitLab Configuration
6. Before review Committer duties • Pull master • Branch from master • Small and atomic change * • Lint • Test • Review ** • Rebase • Push • Open Merge Request ***
7. Small and atomic change How small? • < ~200 LoC (Clojure) or 400 LoC (Go/JS) • < 1h of reading • Atomic changes
8. Review your commits Committer - before push • Review your commits and commit messages. • Squash if needed or rewrite your messages to make them clear and easy to read. Example: git rebase -i HEAD~3 https://git-scm.com/book/en/v2/Git-T ools-Rewriting-History
9. Open Merge Request Committer duties • Review the diff. (again) • Annotate (add comments) • Write a descriptive message • Assign to reviewer
10. Example Merge Request Message
11. Before Review Reviewer duties • Get to know the technology (clojure, scala, go, js, kafka, aerospike…) • Get to know the product (and use case)
12. During Review Reviewer duties • High priority • Read code • (fetch and run locally) • Leave comments * (mark showstoppers) • Add a summary
13. Adding Comments Reviewer • Respectful and punctual. • Suggestive, not decisive • Add references • Be explicit. • Personal preferences / style?
14. • Think • Correctness & Completeness • Naming and Style • Test • Manual tests / MSP namespaces link • Deleted tests • Commented out code • Encapsulation, modularity, coherence • DRY • SOLID principles • Edge cases The Checklist • Security • Race conditions • Documentation • Monitoring • Logging • Configuration • DB queries • Performance • ...
15. Provide Positive Feedback Reviewer duties • Awesome work • Niccccceeeeee! • Love the implementation
16. Large Merge Requests Reviewer • Either ask for breakdown. • Or bite the bullet.
17. During Review Committer duties • Address & resolve comments (discussions) • Be accepting. "Good call. I'll make that change." • Don't take it personally. The review is of the code, not you. • Learning opportunity • Teaching opportunity • "I didn't understand. Can you clarify?" • Fix, rebase, push & notify
18. After Review Committer duties • Merge and delete branch • git bdone • Deploy
19. Timeline Don’t drag Key point: • Lengthy reviews processes are disastrous • Reviewer: Same day / Very high priority
20. Offline Reviews When to go offline? (f2f) • Start online on GL • If too many back-and-forth, go offline (f2f) • At the end document the offline conversation on GL so that next person gets your PoV