Tweet
Tweet

Slide 1

Slide 1 text

Code Review Ran Tavory, Appsflyer Aug 2019 @rantav

Slide 2

Slide 2 text

whoami

Slide 3

Slide 3 text

AppsFlyer

Slide 4

Slide 4 text

Motivation First

Slide 5

Slide 5 text

● For our Customers ● For Us

Slide 6

Slide 6 text

For our customers The immune system “Running Continuous Deployment without an Immune System is playing Russian Roulette”

Slide 7

Slide 7 text

The Immune System Any line of defence will eventually be compromised Reminder: the Bar-Lev line of defence => Solution: Multiple lines of defence

Slide 8

Slide 8 text

The Immune System

Slide 9

Slide 9 text

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

Slide 10

Slide 10 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 11

Slide 11 text

Quality v/s Speed ? NO! Quality is a requirement in order to maintain speed WHY?

Slide 12

Slide 12 text

Exponential Value (and complexity)

Slide 13

Slide 13 text

Software Engineering v/s Programming What is the difference b/w software engineering and programming?

Slide 14

Slide 14 text

What is Software Engineering? Software engineering is what happens to programming when you add time and other programmers. – Russ Cox

Slide 15

Slide 15 text

The Art of Writing is the Rewriting Similar to nobles and poetry 80% of code will be rewritten => Optimize for readability

Slide 16

Slide 16 text

Best Practices

Slide 17

Slide 17 text

The first rule of code review The first rule of code reviews is: ● There are no rules in code reviews.

Slide 18

Slide 18 text

Code Review Objectives ● Correctness ● Maintainability ● Shared knowledge ● Sharpen skills

Slide 19

Slide 19 text

The Flow

Slide 20

Slide 20 text

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

Slide 21

Slide 21 text

Before review Committer duties ● Pull master ● Branch from master ● Small and atomic change * ● Lint ● Test ● Review ** ● Rebase ● Push ● Open Merge Request ***

Slide 22

Slide 22 text

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

Slide 23

Slide 23 text

Open Merge Request Committer duties ● Review the diff. (again) ● Annotate (add comments) ● Write a descriptive message ● Assign to reviewer

Slide 24

Slide 24 text

Small and atomic change How small? ● < ~400 LoC (C/Java/Go/JS/Scala) ● < 1h of reading ● Atomic changes

Slide 25

Slide 25 text

Reviewer The reviewer flow 1. Be informed 2. Review Code 3. Leave Comments 4. … back and forth 5. Approve when Happy

Slide 26

Slide 26 text

Before Review Reviewer duties ● Get to know the technology (language, tools, libraries, databases etc) ● Get to know the product (and use case)

Slide 27

Slide 27 text

During Review Reviewer duties ● High priority ● Read code ● (fetch and run locally) ● Leave comments * (mark showstoppers) ● Add a summary

Slide 28

Slide 28 text

Adding Comments Reviewer ● Respectful and punctual. ● Suggestive, not decisive ● Add references ● Be explicit. ● Personal preferences / style?

Slide 29

Slide 29 text

● 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 ● ...

Slide 30

Slide 30 text

Provide Positive Feedback Reviewer duties ● Awesome work ● Niccccceeeeee! ● Love the implementation

Slide 31

Slide 31 text

Large Merge Requests Reviewer ● Either ask for breakdown. ● Or bite the bullet.

Slide 32

Slide 32 text

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

Slide 33

Slide 33 text

After Review Committer duties ● Merge and delete branch ● git bdone ● Deploy

Slide 34

Slide 34 text

Timeline Don’t drag Key point: ● Lengthy reviews processes are disastrous ● Reviewer: Same day / Very high priority

Slide 35

Slide 35 text

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

Slide 36

Slide 36 text

Guiding Principles

Slide 37

Slide 37 text

Maintainability ≫ Speed

Slide 38

Slide 38 text

Reading ≫ Writing

Slide 39

Slide 39 text

Clear ≫ Clever

Slide 40

Slide 40 text

Simplicity

Slide 41

Slide 41 text

...and Fun Quotes

Slide 42

Slide 42 text

If something unusual is happening, leave evidence for the reader –Brian Kernighan

Slide 43

Slide 43 text

Programs must be written for people to read, and only incidentally for machines to execute. –Hal Abelson and Gerald Sussman

Slide 44

Slide 44 text

The most important skill for a programmer is the ability to effectively communicate ideas. – Gastón Jorquera

Slide 45

Slide 45 text

Simplicity is prerequisite for reliability –Edsger W. Dijkstra

Slide 46

Slide 46 text

Complexity is anything that makes software hard to understand or to Modify. – John Ousterhout

Slide 47

Slide 47 text

Design is the art of arranging code to work today, and be changeable forever. – Sandi Metz

Slide 48

Slide 48 text

Styleguides ● Clojure: https://github.com/bbatsov/clojure-style-guide ● Javascript: https://github.com/airbnb/javascript ● Go: https://github.com/golang/go/wiki/CodeReviewComments

Slide 49

Slide 49 text

References ● https://smartbear.com/SmartBear/media/pdfs/best-kept-secrets-of-peer-code-r eview.pdf ● https://dave.cheney.net/paste/clear-is-better-than-clever.pdf