Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Things I learned after doing 2400 Things I learned after doing 2400 code reviews in 6 months code reviews in 6 months
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Who am I? Who am I?
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Why this talk? Some Why this talk? Some Background Background
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden What is the purpose of code What is the purpose of code reviews reviews ● to investigate the code to find weak spots, to investigate the code to find weak spots, faults, strengths faults, strengths ● to suggest ways to optimize the code for to suggest ways to optimize the code for better performance. better performance. ● to deliver an (ideally) bug-free software that to deliver an (ideally) bug-free software that meets the quality and design standards meets the quality and design standards
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden What is the purpose of code What is the purpose of code reviews reviews ● to share understanding of the code base as team to share understanding of the code base as team members learn from each other members learn from each other ● to maintain a level of consistency in design and to maintain a level of consistency in design and implementation. implementation. ● to identify common defects across the team thus to identify common defects across the team thus reducing rework. reducing rework. ● to offer a different perspective. “Another set of to offer a different perspective. “Another set of eyes” adds objectivity. eyes” adds objectivity.
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Standards / Best Practices Standards / Best Practices ● Objective Objective ● Common across different projects Common across different projects ● Clear Clear ● Documented (code review checklist) Documented (code review checklist) ● Explained / Justified Explained / Justified ● Automated vs Manual Automated vs Manual
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation Build status Build status indicator indicator
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation Rules not allowing a PR to Rules not allowing a PR to be merged be merged
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Preparation Preparation ● Allocate X minutes/time without distraction Allocate X minutes/time without distraction ● Read the specs/ticket description Read the specs/ticket description ● Checkout the code locally Checkout the code locally ● Be focused Be focused ● Revisit the code review checklist once a day Revisit the code review checklist once a day ● Be prepared to see everything! Be prepared to see everything!
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Preparation Preparation ● Be prepared to see everything! Be prepared to see everything!
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Actual Code Review Actual Code Review ● Your work ( as reviewer ) is to Your work ( as reviewer ) is to comment on code not critique comment on code not critique people people ● Be polite to the developer – not Be polite to the developer – not to the code to the code ● Ask questions/ request Ask questions/ request explanations instead of making explanations instead of making statements statements ● But don’t ask “Why” But don’t ask “Why”
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Actual Code Review Actual Code Review ● You have no authority You have no authority because you are the because you are the FUCKING reviewer. FUCKING reviewer. Authority stems from Authority stems from knowledge knowledge ● Don’t act as prima-donna Don’t act as prima-donna ● Respect people with less Respect people with less knowledge and be patient knowledge and be patient ● Review small chunks of Review small chunks of code at a time code at a time
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Actual Code Review Actual Code Review ● Have no fear Have no fear ● Do not compromise Do not compromise ● Be Objective Be Objective ● Give links, articles etc to Give links, articles etc to justify your comment justify your comment ● “ “I have 20 years in java” is I have 20 years in java” is NOT NOT an argument an argument ● Be ready to step back Be ready to step back ● Give kudos (PUBLICLY) Give kudos (PUBLICLY)
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Actual Code Review Actual Code Review ● Read the code Read the code ● How much time to you How much time to you need to understand it? need to understand it? ● Can be easily debugged? Can be easily debugged? ● Is testable? Is testable? ● Design patterns? Design patterns? ● Does it scale? Does it scale? ● Is it secure Is it secure
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Developers Psychology Developers Psychology ● Developers take it personally Developers take it personally
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Developers Psychology Developers Psychology ● Developers... Developers... ● don’t like code reviews don’t like code reviews ● don’t accept they don’t accept they make mistakes. Their make mistakes. Their code is code is by default by default “perfect” “perfect”
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Developers Psychology Developers Psychology ● Developers... Developers... ● don’t want to follow don’t want to follow quality standards quality standards ● don’t want to write don’t want to write (any kind of) tests (any kind of) tests ● are ready to fight are ready to fight endlessly for every endlessly for every comment you make comment you make
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Keep it balanced… Keep it balanced…
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Code Review Maturity Model Code Review Maturity Model Process Tools Review Standards / Guidelines Automation Level 0 Absolute Chaos no established process no tools used no defined standards/guidelines no automation Level 1 Baby Steps on-demand code reviews review tool is partially used no defined standards/guidelines no automation Level 2 Yeoman’s Work reviews as part of the development process review tool is always used / enforced - Integration with CI some standards exist. People are advised to follow the review guidelines Build status is shown on review page Level 3 Quality as First Class Citizen optional rules to merge or not code Integration with Quality measurement tools Guidelines are always followed. Some standards are automatically enforced High level quality status of the review Level 4 The Glory of Code reviews enforce all rules to merge code Integration with Quality measurement tools All Quality standards are automatically enforced Details about quality issues, technical debt, coverage, duplication
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden 5 Takeaways 5 Takeaways ● Reviews is an essential practice Reviews is an essential practice of any Development Process of any Development Process ● Automate as much as possible Automate as much as possible ● Define objective standards Define objective standards ● Be open-minded Be open-minded ● Don’t make it personal Don’t make it personal
Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Questions Questions
ppapapetrou76
rmatsuoka
niftycorp
okuramasafumi
tomokusaba
mackey0225
hitsuji1991
takefumiyoshii
trapezoid
ivanova1991
prof18
miyuki2203
panda_program
shlominoach
davidbonilla
kneath
sferik
sugarenia
aarron
tammyeverts
eileencodes
revolveconf
brad_frost
speakerdeck