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...
For further actions, you may consider blocking this person and/or reporting abuse
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.
wow, this article really got me hooked up. Thanks for the words! Can I translate it to portuguese? What would be the best way to translate it and make it available to portuguese speakers without taking any of your credit? I'm asking it because I don't want to "steal credits" or "steal content", but still I want to make it available to friends and co-workers.
I really like this approach!!
Great article. Thanks a lot for sharing.
Thanks for this great article. We have similar problems in our team and I'm happy every time I can pull suggestions for improvement from different sources like this one.
It sounds like the optimised way to test code but not functional in real life. That level of testing must not be done by developers but QAs who has a higher level of expertise on testing edge cases. It also sounds like a micromanagement technique which is not good. If you don't trust some member, why is that member in the team anyway?
When do I say I don't trust a team member? I actually trust my team an incredible amount.
I found the metaphor of a backseat driver vs a navigator quite interesting. I'm definitely guilty of being a backseat driver. Will keep that in mind when I review code next time. Cheers.