autoscale: true build-lists: true theme: Ostrich, 1
^ Code review is a key part of the software engineering process. I'm going to be talking to you today about making code reviews as effective as possible.
[fit] Justin Spahr-Summers
^ Let me introduce myself. My name is Justin Spahr-Summers, a.k.a., @jspahrsummers on basically every platform.
^ I've worked at smaller companies and larger ones, and been a key contributor to several successful open source projects in the Cocoa community, including Carthage, ReactiveCocoa, the Squirrel update framework, and the Mantle model framework for Objective-C.
^ In total, I've done thousands of code reviews—possibly tens of thousands—in both commercial and open source contexts.
^ Why this talk?
^ Code review is fundamental to our discipline. Without code review (and without other key fundamentals like testing, or CS theory), we're just programming—not engineering. We elevate our craft, and hold each other to a higher standard, through peer reviews.
- Waste everyone's time today
- Waste everyone's time in the future
- Provide a false sense of security
^ Many (I daresay most) teams are already doing some kind of code review. But if it's not effective, what's the point?
^ If your reviews aren't preventing technical debt, aren't improving the end result, and don't provoke good conversation… it's just burning everyone's time for little benefit—and your future self will pay the opportunity cost.
- Minimize technical debt
- Improve the architecture
- Share domain knowledge
- Provide teaching opportunities
^ Submitting a pull request should be the start of a conversation, not the end of one. Use code reviews to achieve all of these goals, and really dig deep into the essence of what you're trying to achieve. The result will benefit everyone!
^ I intentionally do not say that "great reviews prevent bugs." I think bugs should primarily be prevented through type systems, tests, etc., and not rely upon humans to catch before merging. But of course, some reviews will indeed catch some number of bugs.
^ Hopefully we agree on how important it is to make reviews as effective as possible. Let's now discuss how to do that.
^ Before deciding on or describing any process, I like to identify the first principles. Process should follow principles, not the other way around.
^ For code review, the following are my principles.
^ A great code reviewer is a great engineer, because being able to dig in and understand someone else's code, as well as how to improve it, is required all the time when both writing and reviewing code.
^ (Note that it doesn't work the other way—not all great engineers are automatically great reviewers! It's an additional skill, and requires commitment to become good at.)
^ One of the most impactful things you can do is unblock your colleagues. PRs sitting open and unreviewed = lower productivity for everyone.
^ I recommend establishing a maximum turnaround time for code reviews (e.g., 24 hours during the working week). If you're having trouble context switching, or bouncing back and forth between reviews and your own coding, establish a few recurring review times as a habit—like before starting your day, after lunch, and at the end of your day.
^ You may have heard the expression, "you are not your code." I prefer "they are not their code."
^ The code that someone submits should not reflect upon them as a person. Code reviews should never make it personal. It should be about the problem we're trying to solve, and whether this particular version of the change is the best way to do it.
^ Many, many important things get omitted in communication because someone assumed, "this is obvious, I don't need to say it."
^ Always state your assumptions, and overcommunicate. Code authors, explain what you're doing and why! Code reviewers, explain the feedback you're giving, and why it will help!
- Imagine the best version of the code
- Unblock your team
- Criticize code, not people
- Over-explain everything!
^ So just to recap, my first principles are to:
- Imagine the best version of the code as the starting point for a review.
- Unblock teammates and collaborators, to increase everyone's productivity.
- Focus criticisms on code, and not make things personal.
- And over-explain everything. This goes for authors and reviewers.
^ Okay. Let me share the process that I use now—the one I've honed over thousands of code reviews.
^ I approach code reviewing in stages, "outside-in."
^ By starting at the highest level, I can short-circuit my review if I reveal anything that might require a major change to the pull request. There's no point in reviewing each line of code if the architecture is all wrong!
^ Let's start with the intention behind the change, and make sure we agree on what we want this PR to actually accomplish.
^ Every single pull request summary should explain the goal of the PR. Otherwise, how will you as a reviewer evaluate it?
^ The author of a pull request will have the most context on the problem, so it's important that they can explain their goal clearly. And if the explanation doesn't make sense to you as a reviewer, request changes!
^ Here's an informative pull request summary from Christoph Nakazawa on Jest (a JavaScript testing framework). Although it could use a bit of structure (e.g., with headings), it's very clear in its goal…
^ The very first sentence gives you the goal right away—to introduce "a new haste map implementation … which is much more scalable than node-haste." There's some assumed context here, but in this case, it refers to something all of the reviewers will be familiar with.
^ Then, the summary elaborates on why that goal is important. The existing implementation "isn't well designed and not scalable." "This implementation is attempting to [reduce] startup time."
^ Boom. Reviewers now have enough information to evaluate whether the goal is worth achieving (i.e., if this is the right problem to solve).
^ The next thing to figure out is…
^ Presuming you, as the reviewer, understand the goal of the PR, your next responsibility is to determine whether this pull request actually achieves it.
^ For example:
- Is the bug fix or new feature tested?
- How will you know if there's a regression?
^ These are a couple of the questions you (the reviewer) should have, and the author should have satisfying answers for you right there in the PR summary.
^ Here's another pull request on Jest, from Miguel Jiménez Esún. All we need to know from this excerpt is that this is intended to be a performance improvement.
^ And we have evidence that it succeeds at improving performance, because the PR helpfully includes benchmarking results right in the description—along with the process by which anyone can run their own benchmarks.
^ This same PR description includes a picture of automated test coverage after the change. We have evidence that despite the performance improvement, no functionality has regressed. Great!
- Background context
- What the bug or feature is
- What the change achieves
- How the change is tested
- Known limitations or anything still missing
- Request changes if the summary is incomplete!
^ To flesh out the summary a bit more, it should include:
- Background context: any prerequisites necessary to understand the change
- Bug or feature: the goal of the pull request
- What it achieves: similar to, but not the same as, the goal
- How it's tested: include a description of automated tests, or at least a test plan!
- Known limitations: anything that is still to come, so the reviewer gets a picture of where this is heading next
^ One more illustration, this time a pull request I randomly sampled from GitHub Desktop. I like this one because it's adding a new feature to an application, and… what's the best way to demonstrate that that's achieved?
^ A video recording of that feature being used! The author has virtually let the reviewer try the feature themselves, without having to check out and build the code from scratch.
^ As the last step of understanding intent, evaluate (at a glance) whether the changes you see align with the explanation given in the summary. For example, are major architectural shifts explained?
^ Does this PR depend on something else? Does something else depend on this one? Note that individual changes in a stack should still meet a high quality bar.
^ That's our review of the intent. This is a good place to pause and reflect. If the intention of the pull request is not clear, or you disagree with the goal, there's no benefit to reviewing further.
^ But if you're on board so far, now it's time to review the high-level design of the changes.
^ Try to think through the bug or feature described, and mentally design your own solution for it.
^ If you didn't have this PR in front of you, how would you have done it? Does that highlight any gaps in the design you're reviewing?
- Are the architectural choices justified?
- Do you understand it well enough to use or extend?
- Would everyone else be happy to maintain this?
- Does the architecture make your life easier?
^ If the author disappeared tomorrow, would the rest of the team be able to (and want to) work with this?
^ Will this be easy to run in production (whatever that means for your codebase)? Generally, simpler architectures are more maintainable.
- Is the API understandable?
- Does the documentation teach the reader how to use it?
- Is the API conventional?
^ Put yourself in the shoes of an API consumer. If you didn't have this PR in front of you, would you understand how to use it?
^ Remember the principle of "don't assume it's obvious:" what the author finds self-explanatory is often not the case for others!
^ On convention: if in Python, is it Pythonic? If inside a framework with its own conventions, does it match those? Although sometimes necessary, breaking convention introduces cognitive overhead.
^ This is the most subjective part of the whole review, and partly just comes with experience, but here are some heuristics to help you answer this question.
^ Remember these for both quality coding and reviewing!
^ "Always implement things when you actually need them, never when you just foresee that you need them."—Ron Jeffries
^ "Do the simplest thing that could possibly work."
^ In other words, don't over-engineer things. Only make the design as complex as you need to for today's requirements, not the hypothetical future.
^ When the two are in conflict, prioritize simplicity. If something is made easy just for the sake of familiarity, it can actually lead to more complexity, especially if the abstractions are leaky.
^ For example, platforms that try to make concurrency invisible to the developer can end up making things more complex (when bugs inevitably arise), even if it's easier to write the code at first.
^ See Rich Hickey's presentation, Simple Made Easy, as well as my favorite paper, Out of the Tar Pit.
^ Does the API help consumers fall into the "pit of success," where doing the right thing is easy?
^ Are the default choices sensible and safe? Whenever there are multiple options, does the documentation clearly explain the consequences of each? Is the developer led toward the "right" choice?
^ It should be hard to mess things up! Strong static typing can help a lot here, by preventing logic errors and offering guidance to the user.
^ APIs like this can still offer escape hatches, to make "wrong" edge cases possible, but it's not a bad thing if they feel hard to use!
SOLID principles
[.column]
-
Single-responsibility principle each thing should have only one responsibility
-
Open–closed principle behavior should be extensible without modifying code
-
Liskov substitution principle types should be replaceable with subtypes
[.column]
-
Interface segregation principle many specific interfaces are better than one über-interface
-
Dependency inversion principle depend upon abstractions, not concrete implementations
^ Classic software engineering principles, still as relevant as ever.
^ That's our review of the design. It's good to pause again here. If the design has significant flaws that need to be fixed, reviewing the tests and the implementation won't add any value, because they may completely change.
^ If the design mostly looks good, it's time to look at the implemented behavior.
- Tests are additional documentation
- Tests should protect against regressions
- Tests should validate the API contract
- Tests need to be understandable
- Are there missing tests?
- You can request changes!
^ We'll start from the tests. This is sort of the code review analogue to test-driven development—verify that the tests make sense before checking the implementation.
^ Why tests first? They're a form of documentation, and they verify the API does what it says. This means we should look at what the tests are telling us, rather than the implementation.
^ If one of the tests fails, will you know how to investigate and resolve it? Sometimes a simpler test, covering less, is better than a complex test that we cannot hope to debug.
^ Don't wait until later for missing tests—they'll never get added!
- This is the least important part to review!
- Be suspicious of convoluted code
- Would you be able to debug this code?
- DRY: Don't repeat yourself
- Don't reinvent the wheel
- Don't ignore linters and warnings
^ Finally, the implementation. But this is the least important part to review—correctness issues should mostly be caught through type systems and testing, not human reviewers. If the API contract is sensible and there are tests that validate it, any* implementation fulfilling the contract should be basically OK.
^ Does the code achieve its goal in the simplest, most maintainable way possible?
^ Code doing the same thing multiple times should factor that out.
^ Does something already exist to do (part of) what this implementation is trying to do? If you're a particularly helpful code reviewer, you can even offer suggestions for existing APIs, to help the author out!
^ If it ever feels like we're "swimming upstream," where there are flags and code smells left and right, maybe there's a good reason for it! Dig deeper—something that looks convoluted or hard might actually be the wrong thing to do.
^ * of course, there are exceptions; e.g., performance requirements, side effects
^ So just to recap this process…
^ Keep this ordering in mind, so that you can go "outside in." You'll maximize your own efficiency at reviewing changes, and you'll be focusing your feedback on what is most relevant for the PR author.
^ Outside of the process itself, here are just a couple of things to add.
- Always request changes or accept
- Be pragmatic for urgent changes
- Solicit second opinions
- Prioritize your feedback
- Provide concrete suggestions
- Explain the feedback
^ I've been saying "request changes" a lot. That does not necessarily mean "you need to change this." It's really just a signal that the author needs to take some action or provide more information (e.g., respond to a question) before the PR is ready for another review.
^ Likewise, "accept" does not mean "this is 100% fine." You can ask the author to make changes before landing, and we should trust each other enough that this will happen.
^ Be pragmatic: don't hold up critical fixes on a technicality.
^ On contentious topics, CC more people and ask them to weigh in! Make it a group decision, with consensus, instead of an argument to a stalemate.
^ Prioritize your feedback according to what you believe is most important to address ("definitely fix this" vs. "take it or leave it"), and provide suggestions for how the author can do that (e.g., "just a suggestion here").
^ Always remember to justify your feedback, just like you expect the PR itself to be justified. Point out how changing this thing here will lead to a better specific result, or end up improving this other thing over here.
- Each commit should build logically upon the previous
- Clean up after yourself:
git rebase -ihg histedit
- [Stack](http://bentrengrove.com/blog/2020/7/8/how-to-stack-prs-in-github) dependent changes
^ As a pull request author, you can apply all of the advice I've given so far as well! You can make your summaries clear and informative; you can evaluate and improve your design, tests, and implementation just like your reviewers will.
^ Here's one more thing that you, as an author, can do to make your reviewers' lives easier. By breaking down your changes into a logical sequence, they can see the individual parts of the change more clearly, and naturally follow the progression to a complete feature or bug fix.
[.column]
- Imagine the best version of the code
- Unblock your team
- Criticize code, not people
- Over-explain everything!
[.column]
^ So just to recap, my first principles are to:
- Imagine the best version of the code as the starting point for a review.
- Unblock teammates and collaborators, to increase everyone's productivity.
- Focus criticisms on code, and not make things personal.
- And over-explain everything. This goes for authors and reviewers.
^ And my process is to go outside-in, starting by reviewing the intent, then the design, and finishing by reviewing the behavior. I'll stop short at any point if I feel that major changes are necessary—this avoids wasting the reviewer's and the author's time.
Slides and notes available at: github.com/jspahrsummers/effective-code-review
Thanks to Lightricks and Barak Yoresh for inviting me to speak, and to Christoph Nakazawa for sending me some great example PRs!
^ Thank you all! I'm happy to take questions on anything and everything.