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

Code Review Best Practices

Ran Tavory
September 04, 2018

Code Review Best Practices

For AppsFlyer internal teams

Ran Tavory

September 04, 2018
Tweet

More Decks by Ran Tavory

Other Decks in Technology

Transcript

  1. The first rule of code review The first rule of

    code reviews is: • There are no rules in code reviews.
  2. 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
  3. Before review Committer duties • Pull master • Branch from

    master • Small and atomic change * • Lint • Test • Review ** • Rebase • Push • Open Merge Request ***
  4. Small and atomic change How small? • < ~200 LoC

    (Clojure) or 400 LoC (Go/JS) • < 1h of reading • Atomic changes
  5. 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
  6. Open Merge Request Committer duties • Review the diff. (again)

    • Annotate (add comments) • Write a descriptive message • Assign to reviewer
  7. Before Review Reviewer duties • Get to know the technology

    (clojure, scala, go, js, kafka, aerospike…) • Get to know the product (and use case)
  8. During Review Reviewer duties • High priority • Read code

    • (fetch and run locally) • Leave comments * (mark showstoppers) • Add a summary
  9. Adding Comments Reviewer • Respectful and punctual. • Suggestive, not

    decisive • Add references • Be explicit. • Personal preferences / style?
  10. • 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 • ...
  11. 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
  12. Timeline Don’t drag Key point: • Lengthy reviews processes are

    disastrous • Reviewer: Same day / Very high priority
  13. 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