Things I learned after doing 2400 code reviews in 6 months

Things I learned after doing 2400 code reviews in 6 months
Talk presented @ Voxxed Days Bucharest 2017

Transcript

1. 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
2. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Who am I? Who am I?
3. 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
4. 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
5. 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.
6. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden I learned things about… I learned things about… • Standards Standards • Automation Automation • Preparation Preparation • Actual code review/comments Actual code review/comments • Reviewers Psychology Reviewers Psychology • Developers Psychology Developers Psychology • Balance Balance
7. 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
8. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden
9. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation • Pull request builds with branch merging Pull request builds with branch merging • Run Unit tests Run Unit tests • Run Integration tests Run Integration tests • Run Quality analysis Run Quality analysis • Integration Integration • Code review tool + Build tool + Quality tool Code review tool + Build tool + Quality tool • Rejection Rules Rejection Rules
10. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation Build status Build status indicator indicator
11. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation Quality Gate Quality Gate status status Technical Debt Technical Debt delta delta Quality Issues Quality Issues introduced / introduced / fixed fixed
12. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Automation Automation Unit tests Unit tests coverage coverage Integration Integration tests coverage tests coverage Quality issues Quality issues found / found / reported reported
13. 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
14. 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!
15. 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!
16. 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”
17. 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
18. 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)
19. 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
20. 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
21. 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”
22. 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
23. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Keep it balanced… Keep it balanced…
24. 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
25. 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
26. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden
27. Voxxed Days Bucharest – March 2017 Voxxed Days Bucharest – March 2017 Patroklos Papapetrou Patroklos Papapetrou @softwaregarden @softwaregarden Questions Questions