Code Review Best Practices
Download as PPTX, PDF•5 likes•4,706 views
The document discusses best practices for code reviews, emphasizing the importance of ensuring code quality, sharing knowledge, and collaboration. It outlines key considerations such as when to review, who should conduct reviews, and what to look for during the process to avoid common anti-patterns. The ultimate goal is to facilitate understanding and deliver well-functioning code efficiently.
Code Review Best Practices
- 1. — Trisha Gee (@trisha_gee) Developer & Technical Advocate, JetBrains Code Review Best Practices
- 5. Having Opinions On Code Is An Occupational Hazard
- 6. Having Opinions On Code Is An Occupational Requirement
- 7. Are we harder on other people’s code than our own?
- 8. What to look for in a code review
- 10. • Gateway Reviews • Knowledge Sharing • Early Design Feedback Different workflows https://blog.jetbrains.com/upsource/tag/code-review-workflows/
- 11. What should you look for when reviewing code?
- 12. It Depends
- 13. My First Code Review My job is to Find Problems
- 15. Anti-Pattern Design changes when the code works
- 19. Developers hate code reviews
- 20. Code Reviews are a Massive Waste of Time
- 21. 1. Why?
- 22. Ensure code meets standards
- 23. Find bugs
- 24. Share knowledge
- 25. Check code is understandable
- 26. Ensure code does what it’s supposed to
- 29. 2. When?
- 30. • During implementation? • When it’s ready to merge? • After it’s been merged? When do you review?
- 31. • When everyone agrees? • When a gatekeeper agrees? • When all comments are addressed? When is the review complete?
- 32. 3. Who?
- 33. Who reviews the code?
- 34. Who signs it off?
- 35. 4. Where?
- 36. Pairing
- 37. Showing code to a colleague at a computer
- 38. Mob reviewing in a conference room
- 40. In the IDE, checking out a commit or branch
- 41. Using code review software
- 42. 5. What?
- 43. 1. Why 2. When 3. Who 4. Where Requires you to know:
- 44. • Fit with the overall architecture • SOLID principles, Domain Driven Design, Design Patterns or other paradigms of choice • New code follows team’s current practices • Code is in the right place • Code reuse • Over-engineering • Readable code and tests • Testing the right things • Exception error messages • Subtle bugs • Security • Regulatory requirements • Performance • Documentation and/or help files been updated • Spelling, punctuation & grammar on user messages What to look for https://blog.jetbrains.com/upsource/tag/what-to-look-for/
- 45. Human reviewers should be doing what cannot be automated
- 47. Why: Knowledge Sharing Purpose isn’t to reject the code
- 48. Why: Knowledge Sharing Focus is on how easy it is to understand the code
- 49. When: At the end Too late for design
- 50. When: At the end Should have specific checks
- 51. 6. How?
- 52. Automate Everything You Can
- 53. Submitting for review Annotate your code
- 54. Submitting for review Reviews should be small
- 55. Reviewing Should be clear Who is reviewing
- 56. Reviewing Respond in a timely fashion
- 57. Reviewing Checklist of What to look for
- 58. Comments Bear in mind Why, When and What
- 61. Accept or Reject
- 62. Accept or Raise Concern Next steps should be clear
- 63. Making changes Respond in a timely fashion
- 64. Making changes Respond to comments
- 65. Resolving The goal is to accept the review
- 66. Resolving Should be clear Who signs it off
- 69. 1. Why 2. When 3. Who 4. Where 5. What 6. How Clarity Comes From Understanding
- 70. Not to prove how clever you are The Goal Is To Ship The Code
Editor's Notes
- #3: argh my eyes
- #5: And it’s tested. And performance doesn’t really matter
- #8: I think we’re harder on code when we read it than when we write it Reading code is hard We could use some guidelines and ideas
- #9: This is what people automatically think of And this is the first blog post I was asked to write for Upsource
- #10: In fact, turned out to be such a big topic that I wrote a whole series of blog posts, that got turned into a book The more I looked into it the more I realised that this was impossible. No-one could list everything that needs to be checked, no code could pass every check
- #13: The real answer is, it depends One size does not fit all
- #14: STORY: Code Review at UBS - Didn’t know what I should be looking for - Took it as my opportunity to forward my own ideas (e.g. unit testing, clean code)
- #15: Can be solved with automation
- #16: Understand when the review happens
- #17: Inconsistent between reviewers, but even from the same reviewer Happens when there’s no guidelines or goals
- #18: Because reviewing code is hard and boring And because we generally don’t get rewarded for reviewing code
- #19: No understanding of what “Done” looks like I could go on but that is not this talk
- #20: As an author you’re already in a place of vulnerability And as a reviewer it just break your flow and it’s hard to read code you didn’t write. - Stops code going into production - Code doesn't necessarily get better - We don’t get better Code reviews massive waste of time
- #21: Which is why everyone focuses on telling you how important it is to do them, instead of how to do them well
- #23: Couldn’t this be automated? What are the standards?
- #24: Much can be automated but mission critical systems obviously benefit from human reviewers as well Look for gotchas: people experienced in the codebase can see if someone’s doing something that seems right but will have unintended consequences If your code is clean and easy to reason about, if you have good automated tests, you should need to spend very little time on this
- #25: To increase the bus factor
- #26: Not necessarily the same Here one or more people check the code to see if it could theoretically be understood by others
- #27: Can’t be automated
- #28: To evolve the code together
- #29: Could use code reviews to enforce cleaning of tech debt as you go, or migrating a legacy system. Rules like: - Cleaning the method you’re in - addressing any warnings (or suppressed warnings) for this code - also may need rules about what’s committed and how (e.g. formatting changes / warnings changes are a separate commit) You can’t automate any of this!
- #31: This depends upon the goals
- #32: When we’re so bored we give up?
- #33: Who reviews the code? Who’s responsible for signing off? Is a single “no” a veto, or is there another process?
- #34: Is it the same people every time? A set of experts? Or a rotating pool? Do people have areas of expertise? Assignment can be automatic based on: - who’s the author - which code was touched - which branch was touched Watchers vs reviewers
- #35: One person? A committee? Is it all or nothing? Or a veto system? How are deadlocks resolved?
- #36: Where the team is located and where the reviewers are located impacts your code review process And where you do the code review
- #37: My favourite
- #38: If you’re located in the same office Forces you to be nice
- #39: Forces input from many Forces resolution of deadlock?
- #40: Via skype or slack or hangouts Don’t have to be collocated but have to have overlapping timezones
- #41: Can be fully async, works for remote and unfriendly timezones
- #42: Also includes GitHub pull requests Also fully async
- #43: To look for in a code review We can only identify this once we know the answers to the other questions And it shouldn’t be a wishlist of everything
- #45: For example, if the code is related to Orders, is it in the Order Service? If so, should it be refactored to a more reusable pattern, or is this acceptable at this stage? How does the team balance considerations of reusability with YAGNI? Are confusing sections of code either documented, commented, or covered by understandable tests (according to team preference)? Is the code going to accidentally point at the test database, or is there a hardcoded stub that should be swapped out for a real service?
- #47: System constraints: - Internal systems hosted on safe hardware don’t need the same level of security checks - Web applications don’t need (the same) low latency performance - If you do have regulatory requirements these should be worked into the code review (or automated) – e.g. audit
- #50: Checking design needs to be factored into the process some other way Like up front design Or a whiteboarding session
- #51: There shouldn’t be any surprises
- #53: Formatting checks, applying formatting The build Testing - Unit, integration, end to end, performance, security Deployment
- #57: Use due dates if possible
- #58: And priority Aligned with code review goals and constraints MOSCOW - Must Should Could Would
- #59: Why are we doing this code review When is it being done What are we looking for
- #60: Be nice. If you’re nice, the author is more likely to listen Comments should relate to the code, not the author Can also leave praise
- #61: What actions need to be taken What priority are they (labels, colours)
- #62: What actions need to be taken What priority are they (labels, colours)
- #63: Concerns listed and prioritised Can’t reject without comments MOSCOW When you know Why, When, Who, Where, What it should be clear whether it’s accepted or not And what the next steps are
- #64: Minimise cognitive load/context switching Prioritised feedback should make this easier And always respond to questions as fast as you can (even if it’s I’ll get back to you) – don’t be the ghost reviewer
- #65: And resolve answered discussions
- #66: Resolve as fast as possible. The goal is to accept the code, not to make it wait in line
- #67: Who decides when the code is good to go? All reviewers or some? Any power of veto
- #68: If next steps / actions were clear, if priorities were clear, it should be easy to understand when the review is good to go
- #69: Objectives = consistency Objectives means fewer surprises, greater consistency Also can lead to code that meets those requirements before entering review
- #70: You can’t have sensible answers to any of these without knowing why first Only when you’ve answered the first 4 can you sensibly know what to look for and create guidelines on how to do the code review. If you have no control over the code review process but don’t know the asnwers to these questions, ask someone.
- #71: Does it do what it’s supposed to do? Is there anything horribly wrong with it? Does adding this feature/fixing this bug add more value than any debt introduced by maintaining this code?