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

Code Review @ AppsFlyer

Code Review @ AppsFlyer

Ran Tavory

August 16, 2018
Tweet

More Decks by Ran Tavory

Other Decks in Technology

Transcript

  1. The Immune System What is the Immune System? • The

    line of defence will always be broken => Solution: Multiple lines of defence
  2. The Immune System Code Review is part of the immune

    system • Coding ◦ Code Review ◦ ... • Monitoring ◦ ... • Instrumentation ◦ ... • KPI Monitoring ◦ ...
  3. 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
  4. 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?
  5. 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
  6. Reviewer The reviewer flow 1. Be informed 2. Review Code

    3. Leave Comments 4. … back and forth 5. Approve when Happy
  7. 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
  8. 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
  9. 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
  10. What size is ideal? • 100 - 300 LoC (research

    on C code, for Clojure - shorter) • 30-60 minutes
  11. Not Covered Today • Linters • Tests and Coverage •

    CI with CR • Design Review • Readability and Ownership • CR Checklists (?)