I'm a big advocate for code reviews. I've written about my experiences reviewing code, including lists of dos and don'ts, how to survive your first code review, and why we should review code at all.
My views have been mainly shaped by my experience with a particular kind of code review - the pull request. Like many devs, I've grown accustomed to the pull request model to help facilitate reviews. While it isn't perfect, I've always found it to be a great tool and overall an improvement to formal code review processes or reviews over email.
Recently, though, I've been seeing thoughts expressing a different sentiment. Many devs are starting to become frustrated with the pull request model. There is a sense that pull requests actually might make it too easy to comment on a line of code. This ease might be leading to the conflicts and hyper-zealous commenting that frustrates many in our industry.
Another common critique is that the pull requests encourage reviewers to only look at the code. They never even pull down the changes they are reviewing! To be completely transparent, I'm guilty of that myself.
After reading the article and seeing some related ideas via Twitter, I wondered: are there better ways that I could review code? What could I do differently?
After asking, I've decided to start making some changes in how I review code. They've been helping me recently, so I think they could help you and your team as well.
Commenting on Code vs. Collaborating on Code
One of the top thoughts that made me start questioning how I review code comes from the article above:
Pull requests are an improvement on working alone. But not on working together.
That struck a chord. Indeed, pull requests aren't the primary way to collaborate with people after all.
Collaborating on code is vital to a team's ability to maintain a codebase. If a single member of a team is left to write an entire codebase without working with other team members, there is a high probability that the rest of the team won't be able to contribute to it after a while. Worse, if left alone, a single team member might even get things wrong or be unable to solve a problem. Therefore, collaborating early and often is a good idea.
The next question that arises is this: how does a team collaborate on code? Further, can a team collaborate too early? Too often?
Many teams seem to have concluded that a pull request is a great place to have collaboration. I don't think they are wrong. First, a team member feels ready to share a version of their code with the rest of the team for direct feedback. Then, like an author writing a book, they submit it for a first review to get feedback. The rest act as editors, ready to help get the draft ready to print.
There isn't anything wrong with teams seeing pull requests as a place for collaborating. The problem is when teams think collaboration is restricted only to the code review process.
Teams should be working together on problems, ideas, and code well before code is submitted for review. Mentors should be advising mentees, peers should be offering help to each other, and pair programming or debugging sessions should be routine. If an author were to send the first draft to their editor without disclosing what the book is about, it would make it harder for the editor. The editor now has more they need to understand to give useful feedback. As a result, the publication date is likely to get pushed later than anticipated.
Genuine collaboration is more than just leaving a comment or a suggestion. It's discussing alternative solutions, prodding at requirements, and having real-time feedback loops with your team.
I will be working towards making this form of collaboration more common within my team. I'm hoping to move us beyond collaboration via comments and get back to real-time collaboration.
Run. The. Code.
Commenting on and rejecting a pull request without ever running or stepping through the changes made sounds silly when said directly. But many developers - including me! - do this all the time.
As engineers, we shouldn't evaluate code only by how "clean" it is or how wonderfully abstracted it is. Instead, we still need to measure code by how well it does what it is supposed to do - deliver some value when it runs.
To drive this point home, when was the last time you looked at Google's source code for Google Search? If you did see it, would it change how well you think it works as a product? Probably not.
I'm not trying to be hyper-reductionistic here. The quality of code does matter! It should be clean, crisp, mutable, etc.
But part of what makes that code good is how well it solves a problem. Does it do what it needs to do to meet requirements? Will the end product be better? At the least, all code needs to make the end product better.
What is the best way to see if the code makes the product better? You have to run it. Treat a pull request as a chance to do a mini-demo of the added or fixed functionality. Use it. Poke at it. Heck, even test it yourself!
I've been incredibly guilty about this. I tend to trust our automation that the code changes produced the desired behavior changes without observing the behavior myself! I want to start changing this and running code more frequently.
More Navigating and Less Backseat Driving
When a problem (or even a difference of opinion) is spotted in code, it can be easy for a reviewer to recommend a concrete change right then and there. For example, they might include a block of code in their comment or add a GitHub suggestion of their improved implementation. They might even include citations and links.
And they do this for every issue they see.
While there are times adding such comments can be appropriate, it often can feel like the reviewer is attempting to take control. They are dictating what the code should be rather than providing feedback on the written code. The reviewer is giving directions as if they were a backseat driver. And no one likes backseat drivers.
Great code reviewers know the difference between being a backseat driver and being a navigator. Both give directions, but one has the destination in mind while the other is trying to control the driver. A navigator knows multiple ways to reach a destination, but a backseat driver complains when the driver takes Martin Street instead of Hampton street - it could have saved one whole minute!
What does a navigator look like in a code review? Instead of focusing on how to change the code, they are focused on why the code needs changing. They care more about the final outcome rather than the concrete classes that are written. The navigator is primarily focused on understanding where they need to go and to help them get there.
I want to be more of a navigator. Not every single detail needs commenting, and not every difference of opinion needs to be shared. I want to review code in such a way that I'm helping rather than controlling.
The goal of all of this is really more than becoming a better code reviewer. It's about becoming a better teammate. Knowledge work isn't just an individual grinding away on an intense thought or task anymore. Knowledge work - or at least meaningful knowledge work - tends to be accomplished by teams.
And I want to be a great teammate.
Originally published at https://dangoslen.me.
Photo by Francesco Gallarotti on Unsplash
Top comments (27)
I think emphasizing pulling the code and running it as a pre-req to engaging in a code review is increasing the friction of the code review sufficiently that you're going to get less engagement on a given PR across the team. This seems bad to me.
In my opinion, an ideal PR gets lots of contact across the team and robust conversation around proposed changes. A high quality PR should include automated tests to validate the work, and sure, there should be some manual verification of what was done, but I think the author can own most of this.
Oftentimes, I've been on projects where reviewing the PR means that you, as reviewer, are now also tapped for questions and follow up work on that bit of code. Later, when we look at
git blame(terribly named) for further development, the reviewer listed is someone who may get tagged in.So, you should run the code, because you might end up having to pick up the baton to fix or extend it next. Ideally, there is no single author of the code base. A PR is often a good point for spreading the responsibility and know-how. And spending some time as a reviewer to manually verify functionality can often catch things the bleary-eyed author missed. Even assumptions in automated tests can be wrong.
Another aspect: There's a such thing as too much engagement on a PR. Not everything needs the whole team dog-piling on it. Not everyone needs to feel required to robustly converse about every change. Most PRs, IMO, should be small enough for just one other set of eyes. (Not that I've always managed to adhere to that)
Potentially, though my experience is the opposite. I'm more engaging after I pull down code and poke at it in my IDE and try to run it than just commenting on a webpage.
Also, I wonder: is engagement on a pull request the right thing to optimize for? Shouldn't we optimize for the best solution or the best code?
Yeah, I would agree the depth of engagement is higher when you give it that level of attention. And I definitely would prefer my teammates to dig deep and run the code, especially on particularly complex code changes.
That's a great question. I don't think it's the only the thing you optimize for. But I think it's generally better to get broad exposure, especially on significant changes. This doesn't only benefit the quality of code going into the codebase, but also is an opportunity for the team to keep up to date on the latest changes and to engage in discussion and learn from each other.
The trouble with the PR model is that it is done at the end of the development cycle for that feature/bug fix.
What if the result of the code review was more important than "why did you format the code this way? Why did you name that method XYZ? ABC would be better to represent its true purpose" - case A.
What if the result of the code review was something like "why did you write that entire functionality from scratch when we already have 95% of it already implemented in production proven classes?" - case B.
Trust me, I've experienced too many "case B" OMG moments to dare to think about.
The PR model is great for situations like case A but is completely inappropriate for picking up major architectural issues like case B. If architectural issues are picked up by a PR then it's probably at the end of a sprint and at that stage it's waaaaay too late for the dev to go back and rewrite the code in a way that doesn't screw up what might have previously been a good, lean architecture. So the pressure is on to accept the PR and commit the company down the path of unmaintainable bloatware with a crappy architecture.
When you kick up a stink and try to knock back PRs based on attempting to promote "maintainable code for the future viability of the company" other devs and project managers frame you as some kind of "productivity blocker".
Eventually junior devs are scared to ask you to do code reviews and then further down the track, incrementally, sprint by sprint the company's software becomes the bag of crap you predicted it would become and the team is under pressure from management because users start complaining that the software is slow, unreliable, annoying to use etc.,
What's more important is a design review before devs start writing the code to implement a feature or fix a bug. Maybe they should have to present to the group what they're planning to do before they run away to their rabbit hole and reappear 7 days later with a big bowl of spaghetti that, due to the PR model's 'end of sprint' scheduling, someone will be pressured into accepting... so that "we can get it into testing before the sprint ends".
Insisting on a "Design Review" (DR) before implementation means that a key function of the "Pull Request" (PR) at the end would be to confirm how close the dev's implementation is to the agreed upon design. i.e. not just some relatively unimportant "feel good" B.S. to make sure that their "opening curlies" are the right place.
Waiting until the end of the development cycle is indeed a bad idea.
Our team tries to do various forms of pairing on a feature/bug before we ever get to a pull request. We also encourage opening them very early in the development cycle rather than waiting until the end.
Thanks for reading!
It's an important point you bring up about making the review-experience as comfortable as possible for the person receiving the feedback as well.
Personally I've found that people vary greatly on the type of feedback they want:
So yea, I'd add the following recommendations:
Big +1 for pair programming. The quality of the code that came from the pair-programming team I worked with was higher than any other team I've been on.
It isn't just about another pair of eyes to catch mistakes; it's about the fact that two heads are nearly always better than one, and the code/solutions that came from pairs were typically more sound/robust than code/solutions from solo devs.
yea I love the idea of pairing, and haven't gotten to do it as much as I'd like; mostly people feel uncomfortable initially to have someone else look over their thinking-process and would rather work alone and struggle through things instead. 😅
Hey Joel! Thanks for reading.
100% agree with your thoughts on this. I espeically agree with 1 and 3. Having a team understood expectation about what happens during a code review can solve (or at least help solve) a lot of the problems in the review process. Same goes for pairing (which our team does currently).
I certianly can't write about every aspect of good reviews or team culture in one article, so I'm glad you added these comments.
certainly a herculean task, but thank you for sharing your thoughts and starting this conversation. ❤️
If you can set contributors up for success in your project by providing good test coverage and existing suggested best practices, it is worthwhile to do so. With this, adding test cases for a PR’s affected experiences is a reasonable ask for contributors. Ask them to provide first-hand recording of the working experiences for the happy path and sad path, as well as unit/integration/E2E tests to cover the new work.
It does not require you to pull the code down to try it in order to approve it, because you trust your test infra, and it encourages contribution from others for the same reason, because they trust your test infra.
Hey Brandon! Thanks for reading.
I'm not saying to not trust the tests. The tests might be 100% correct.
I'm trying to simply advocate for knowing what the code's actual behaviors are when reviewing code. The number of times a behavior is different than the requirements written but the tests passed are enough for me to want to check on important code changes.
Is that every code change? Probably not. I can see how that wasn't clear in my article though in that regard so thanks for brining it up.
I just don't know if this is practical. We have a 5 man team and require 2 approvals on most PRs. We can see anywhere from 0-5 PR's in a day. On a bad day I'll be doing nothing but PRs. And it could lock up all 5 developers if they examine them at the same time. But, there could be value in what you are saying, perhaps having a some PR's reviewed more deeply by a second developer, etc.
if you don't mind, what kinds of metrics is your team tracking to keep an eye on things? It sounds like you'll are a lean team and working hard on some ambitious goals, but if you're not keeping an eye on defect rate, lead-times or some kind of proxy metrics then it's easy to get trapped in a local-maxima of constant fire-fighting.
PR based workflow might actually be overkill for a small team though, so you might consider either:
mainI guess this article is about front-end web development, game development or any sector where you actually need to pull and run the code to really see what it does. In backend or library dev the automated tests added in the PR should be sufficient to have a full picture and if they aren't it's an easy comment to make.
Great feedback, although it takes quite a lot of time to run every single PR manually, that's why we have automations.
The "how" vs "why" is interesting, I'll keep it in mind. What I usually do it why + how, when I know of a way to solve an issue with actual code example, I assume it helps the author, but it shouldn't replace the "why", indeed.
It sounds like the PRs in your work place may be quite big routinely. The smaller the PR, the less attention to detail you need when reviewing. I find demoing a bigger feature to the rest of the devs early on is quite effective (code, or the initial draft of how it'll work).
I definitely agree it's a tool, to be used in addition to pairing or using other tools helps deliver the requirements and high quality code.
A great post indeed, have been trying to put some of them in practice. Have always been skeptic about this kind of collaborating where the collab happens after major chunk of work is done, there needs to be concurrent feedback cycles happening along with being involved in the code (by testing it yourself). It helps get more accountability over the code that's being pushed -> better in the long run.
That said, thanks Dan for giving fresh new ideas and @thepracticaldev to put it on Top 10's radar to help more people. ❤
Great read, thanks for sharing. My personal experience: When I asked a senior dev on our team what he was looking for in PRs and how he approached it, part of his answer was 'check out the code and run it locally, play around with it'. In my opinion, this also incentivizes you to grasp the whole component/file/module/... instead of just looking at the changes. It takes time, sure, but it's better to invest that time sooner than later.