PRE-COMMIT
Pro
Quality ensured before merge
All code must be reviewed
Cons
Productivity impact
Back & forth Hell ™
14
Slide 15
Slide 15 text
POST-COMMIT
Pro
Continuous development
Limit git conflicts
Cons
Bug can ship to production
Resolution can become non trivial afterwards
15
Slide 16
Slide 16 text
OPTIONAL CODE REVIEWS
by commit author
by commit length
by files modified
random checks
… ?
16
Slide 17
Slide 17 text
PAIR REVIEW WITH ANOTHER DEVELOPER
Provide a single feedack
Compare different viewpoints
Tone down negative feedbacks
17
Slide 18
Slide 18 text
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
Slide 19
Slide 19 text
HOW ?
THE DOS AND DONTS OF CODE REVIEW
19
Slide 20
Slide 20 text
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
Slide 21
Slide 21 text
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
Slide 22
Slide 22 text
COMMENTING ON ISSUES
“If you can't understand that, then yes, you're crazy. Or just terminally stupid.”
— Linus
22
Slide 23
Slide 23 text
BE PRECISE : WHAT IS THE PROBLEM ?
“THIS CODE IS BAD !” ✗
“THIS LINE COULD CAUSE A MEMORY LEAK…” ✔
23
Slide 24
Slide 24 text
ARGUMENT : WHY IS IT A PROBLEM ?
“OBVIOUSLY!” ✗
“… BECAUSE THE ACTIVITY REFERENCE IS RETAINED…” ✔
24
Slide 25
Slide 25 text
BE HELPFUL : HOW TO FIX THE PROBLEM ?
“DEAL WITH IT!” ✗
“… YOU COULD INSTEAD USE A WEAKREFERENCE.” ✔
25
Slide 26
Slide 26 text
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
Slide 27
Slide 27 text
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
Slide 28
Slide 28 text
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
Slide 29
Slide 29 text
RECEIVING CRITIQUE
Take a step back
Stay humble
Motivate your choices
Accept the critique
(even if it means more work for you)
29
Slide 30
Slide 30 text
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