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

Code Review (Navy Hackathon)

Code Review (Navy Hackathon)

Why and how to code review.
Presented at the navy hackathon Aug 2019

Ran Tavory

August 07, 2019
Tweet

More Decks by Ran Tavory

Other Decks in Programming

Transcript

  1. The Immune System Any line of defence will eventually be

    compromised Reminder: the Bar-Lev line of defence => Solution: Multiple lines of defence
  2. Code review - For us • Improve our skill ◦

    Great way to Teach ◦ Great way to Learn • Maintain Coding conventions • Find Defects early • The Bus Factor: More devs familiar with the code ◦ At least two principle
  3. 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?
  4. What is Software Engineering? Software engineering is what happens to

    programming when you add time and other programmers. – Russ Cox
  5. The Art of Writing is the Rewriting Similar to nobles

    and poetry 80% of code will be rewritten => Optimize for readability
  6. The first rule of code review The first rule of

    code reviews is: • There are no rules in code reviews.
  7. Committer The committer’s flow 1. Create a branch 2. Push

    to GitHub/GitLab/... 3. Create Merge Request 4. Assign to approver 5. … back and forth ... 6. Approval 7. Merge
  8. Before review Committer duties • Pull master • Branch from

    master • Small and atomic change * • Lint • Test • Review ** • Rebase • Push • Open Merge Request ***
  9. 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
  10. Open Merge Request Committer duties • Review the diff. (again)

    • Annotate (add comments) • Write a descriptive message • Assign to reviewer
  11. Small and atomic change How small? • < ~400 LoC

    (C/Java/Go/JS/Scala) • < 1h of reading • Atomic changes
  12. Reviewer The reviewer flow 1. Be informed 2. Review Code

    3. Leave Comments 4. … back and forth 5. Approve when Happy
  13. Before Review Reviewer duties • Get to know the technology

    (language, tools, libraries, databases etc) • Get to know the product (and use case)
  14. During Review Reviewer duties • High priority • Read code

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

    decisive • Add references • Be explicit. • Personal preferences / style?
  16. • Think • Correctness & Completeness • Naming and Style

    • Test added/modified • 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 • ...
  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. Timeline Don’t drag Key point: • Lengthy reviews processes are

    disastrous • Reviewer: Same day / Very high priority
  19. 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
  20. Programs must be written for people to read, and only

    incidentally for machines to execute. –Hal Abelson and Gerald Sussman
  21. The most important skill for a programmer is the ability

    to effectively communicate ideas. – Gastón Jorquera
  22. Design is the art of arranging code to work today,

    and be changeable forever. – Sandi Metz