Zen Code Reviews

Transcript

1. ZEN CODE REVIEWS THE WAY TOWARDS PAINLESS CODE REVIEWS Droidcon Berlin 2017 1
2. ABOUT… XAVIER F. GOUCHET ANDROID ARCHITECT AT ‘MR TOOLS’ / CI ADMIN / UT ADVOCATE ON ALL SOCIAL NETWORKS @XGOUCHET 2.1
3. WHO USES CODE REVIEW HERE ? ♫ Put your hands in the air ♪ — Placebo 3
4. CODE REVIEWS @ DEEZER 18 Android developers 3 main repositories Varied experiences and skills 4
5. WHY ? THE PURPOSES OF CODE REVIEWS 5
6. FIND DEFECTS EARLY Logic fallacies Typos, spelling Edge cases Potential technical debt 6
7. HARMONIZE THE CODE BASE Code style Naming conventions Architecture Libraries 7
8. GENERATE DISCUSSION AROUND… Choices Algorithm Data Structure Architecture Libraries Features Best practices 8
9. TEAM BUILDING Cohesion Trust Lower “Linkedin factor” Asynchronous Pair Programming 9
10. SHARE KNOWLEDGE Onboard junior / new developers Everyone has something to teach Everyone has something to learn 10
11. DOWNSIDES OF CODE REVIEW More time spent per ticket / less time spent delivering feature Senior developer frustration 11
12. WHAT ? THE DIFFERENT TYPES OF CODE REVIEWS 12
13. “AUTOMATIC” CODE REVIEW Static Analysis Android Lints Findbugs, Checkstyle, PMD Detekt, ktlint Tests (Unit, Integrated, Functionnal) 13
14. PRE-COMMIT Pro Quality ensured before merge All code must be reviewed Cons Productivity impact Back & forth Hell ™ 14
15. POST-COMMIT Pro Continuous development Limit git conflicts Cons Bug can ship to production Resolution can become non trivial afterwards 15
16. OPTIONAL CODE REVIEWS by commit author by commit length by files modified random checks … ? 16
17. PAIR REVIEW WITH ANOTHER DEVELOPER Provide a single feedack Compare different viewpoints Tone down negative feedbacks 17
18. CODE REVIEW MEETING WITH THE AUTHOR 3 to 7 attendees Only for non trivial reviews Mentor the author Get in depth view Discuss alternatives Immediate feedback 18
19. HOW ? THE DOS AND DONTS OF CODE REVIEW 19
20. WRITING REVIEWABLE CODE Keep the commits short git add --patch / git add -p Comment the code Use a proper commit message CR Brief for non trivial commits Link to the ticket 20
21. BEFORE SUBMITTING, REVIEW YOUR OWN CODE Find easy to spot issues Typos Commented code Duplicates / possible refactoring Copy / Paste errors Take a step back on your code 21
22. COMMENTING ON ISSUES “If you can't understand that, then yes, you're crazy. Or just terminally stupid.” — Linus 22
23. BE PRECISE : WHAT IS THE PROBLEM ? “THIS CODE IS BAD !” ✗ “THIS LINE COULD CAUSE A MEMORY LEAK…” ✔ 23
24. ARGUMENT : WHY IS IT A PROBLEM ? “OBVIOUSLY!” ✗ “… BECAUSE THE ACTIVITY REFERENCE IS RETAINED…” ✔ 24
25. BE HELPFUL : HOW TO FIX THE PROBLEM ? “DEAL WITH IT!” ✗ “… YOU COULD INSTEAD USE A WEAKREFERENCE.” ✔ 25
26. DEFINE CRITICITY : HOW IMPORTANT IS THE ISSUE ? “…” ✗ “IT’S NOT CRITICAL, BUT MUST BE FIXED IN A LATER COMMIT BEFORE THE RELEASE CANDIDATE NEXT WEEK.” ✔ 26
27. EXAMPLES “The name of this variable is ambiguous. You could rename it as …” “I don’t think this method should be in this class because it’s outside of the class’s responsibility.” “This rule has many edge cases, you should add more unit tests.” 27
28. VOTING DOWN Avoid blocking commits unnecessarily Always make the reasons clear Make a full review If an issue has already been raised, don't comment just to say “+1” 28
29. RECEIVING CRITIQUE Take a step back Stay humble Motivate your choices Accept the critique (even if it means more work for you) 29
30. OVERALL BEHAVIOR Stay open minded Make it about the code, not the people Don't start the flame war Share your knowledge Give as much as you receive 30
31. GOING FURTHER ? CODEREVIEW.STACKEXCHANGE.COM 31
32. THANKS FOR YOUR ATTENTION ANY QUESTION ? 32