6 ms·

Code Review Handbook

118 points by Sevii 4y ago

difflens 4y ago
Nice article! I agree with all your points, they closely mirror my experience too!
- kelsolaar 4y ago
  Same here, there is something reassuring to see that it is quite common!
anshumankmr 4y ago
If I had a dollar for each time a merge request was reviewed by one of my peers and all they said "it looks good to me", I would have a small fortune.
- m463 4y ago
  Weren't you paid for creating those reviews? :)
onion2k 4y ago
The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them on file saves if possible). Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR. Force pushes and skipping hooks should be vigorously discouraged. That way badly formatted code shouldn't make it to the repo and your PRs can focus on more important things. Obviously this is really hard to implement on a project that's old and has thousands of linter errors. If you didn't set it up on a greenfield project right at the start you can't really complain too much though.
- DangitBobby 4y ago
  > Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR I'd rather people check in their WIP than have it sitting on their machine. Let GH actions worry about whether the code is formatted correctly.
- gombosg 4y ago
  I think that often it's not just coding style (yes, a linter should catch indentation errors), but coding conventions/requirements to enforce an idiomatic code structure in the code base. Linters can't catch that. E.g. how to organize files, where to put business logic, how tests are structured, which libraries to use, how to handle non-blocking or concurrent code, errors, code documentation etc.
- allset_ 4y ago
  Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.
  - onion2k 4y ago
    I don't think I'd call any of those things "style". Those things are the code itself, not the style it's written in. They're definitely things that PR reviewers should be looking at.
doix 4y ago
It's interesting that the author puts "downloading the code" in the exhaustive section. Is it really that much work to run git checkout? I routinely checkout code in PRs if I'm not familiar with that part of the codebase. I can navigate the codebase much quicker in my editor than in GitHub. Although GitHub is actually getting there with the semantic analysis of code and finding references and stuff, sometimes I just need to open 5 files in different splits to figure out what is going on.
- eastbound 4y ago
  On bad projects (which happens often now that node_modules directories are often gigabytes big), IntelliJ IDEA is super-long to index, which is required before being able to Cmd+Click on functions. And we have M1. But this is an example where usability impacts the author’s judgement. For me, UX testing is part of the initial step of code reviews, no technical review is necessary if we end up deciding to implement the story another way.
  - gernb 4y ago
    Well, if the code reviewers had correctly disallowed adding all those deps to node_modules then you wouldn't be in this situation. :P I'm only half joking. :P
- noam_k 4y ago
  I think another factor here is if the reviewer is working on the same code base and the project has a complex environment to set up (e.g., a library compiled with custom flags), then `git stash` may not be enough to checkout the branch and run tests. (I am unfortunately guilty of creating some projects like that myself...) Docker can help with these cases, but not all projects are docker-oriented. I personally like running the code myself when reviewing, it's just not always practical.
  - datavirtue 4y ago
    Yeah, reality: no one is going to download code and try to run it. It would take them all day to do code reviews. You are supposed to go live with your buggy shit soon, that's what the business values. Once it's live it gets tossed over the wall and the maintenance process begins where lower paid workers tinker with it until they have the skills to code-and-bail also.
allset_ 4y ago
I think I disagree with the emphasis on asking questions. Not because questions shouldn't be asked, but because they should be asked much earlier in the process than the PR stage. Every feature should at a minimum have a brief doc for the requirements and high level design. If you haven't read that and are reviewing the PR for anything other than style, you need to take a step back and go read that doc first. In my opinion, the levels are: 1. Barely-existent review: The PR author is the SME, I'm only reviewing and LGTMing this because peer review is needed to submit the change. I might notice a typo or something along those lines, and will call out if they're being lazy and skipping tests. 2. Functionality-focused change: Does the code meet the objective and do the tests adequately cover the change? 3. In-depth review: Does the code address the problem in an efficient way that is not likely to cause the team future pain?
imiric 4y ago
It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like to push back on this: > Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code. Reviewing code gives you an opportunity to get familiar with other parts of the code base, understand why a change or fix is done, and collaborate with others to make an overall better improvement to the product. It gets frustrating when egos get involved, and people take criticism too personally, or point out improvements for the sake of demonstrating their knowledge. In best working environments, code is judged without a bias or relation to who produced it, but as an inert output of the collective team. I would actually like to see a feature in code review tools to submit merge requests anonymously, just so that the author isn't a prevalent factor while reviewing. :) I usually dedicate a few hours, maybe even half a day, to reviews. Definitely don't look at it as a chore to put off, but as a crucial part of your responsibilities as a developer.
- joshka 4y ago
  If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?
  - imiric 4y ago
    It depends. If your employer is a company that values good software development practices, then they wouldn't judge you based on the LOCs you produce, but on the overall value you provide to the team, product and company. If they're not, then most of what we're discussing here is not very relevant, since they likely don't follow other good practices either. If you're in that situation, then you have a few options: if you value the product and company, promote these best practices so that the company can attract and keep good developers, which ultimately benefits the product. If you don't, or they won't listen, or you don't have the authority to change things, then you can probably forget about following good practices, and just keep working based on the metrics they do value. Or, I'd strongly suggest, just find a better working environment. :)
marcyb5st 4y ago
I also blame the tooling a bit. After using Critique [1] internally at Google all the other tools/implementations/UIs feel very confusing. And I'm not speaking with lack of integration with other tools like coverage, linters, spell checks, error-prone constructs analysis, ... but simply the UI. Additionally within Google (or at least the teams I was in) really tries to chunk changes in small pieces. For instance, adding a feature always entails something like: 1 CL to introduce the flag that will enable/ disable the feature, at least 1 CL that actually adds the logic and tests, 1 that enables the flag in the deployment. Not sure if that's due to the monorepo or more of a culture thing, but that MO is not that common in my personal experience [1] https://abseil.io/resources/swe-book/html/ch19.html https://abseil.io/resources/swe-book/html/ch19.html
- codeapprove 4y ago
  I agree that the tooling is very underrated! It’s strange that programmers spend so much effort choosing their code writing environment (IDE, etc) but just take what they’re given when it comes to code reviewing (mostly GitHub PRs) If you’re missing Critique, allow me to shamelessly plug CodeApprove: https://codeapprove.com https://codeapprove.com Just like Critique, it’s a code review tool for power users that really lets you focus on resolving every discussion.
  - marcyb5st 4y ago
    Thanks! Will definitely have a look!
  - rob74 4y ago
    Well, you can decide more or less by yourself what tools you use for writing code, but the code review tool is something everyone needs to agree upon (and then convince management to change), so of course it has more inertia. I'd love to use something else than Jira for issue tracking too, but if the whole company uses it...
    - omarelbie 4y ago
      What would you go with instead of Jira? At a small startup here so there's still hope
airocker 4y ago
I tried to get code reviews to be done after merge. Nobody wanted to do it.
- quickthrower2 4y ago
  Optimistic rollups I guess
- jbn 4y ago
  and if you do do it (speaking from experience) people get offended that someone be looking at their code (as if existence in source control guaranteed absence of bugs...). It is all the more surprising since simple inspection of never-reviewed code usually has a lot of low-hanging fruit in terms of bugs.
azangru 4y ago
A handbook? Isn't it more like a hand article? The pdf from 2018 is 6 pages long.
azangru 4y ago
> Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. Does not add a lot to your bottom line? This is the mindset of isolated individual contributors rather than members of a single team that work on a given feature of the same product. Isn't this precisely what various agile approaches (scrum or XP in particular) are fighting against?
- yakshaving_jgt 4y ago
  > This is the mindset of isolated individual contributors rather than members of a single team that work on a given feature of the same product. From experience, I think this is the more common scenario, unfortunately. > Isn't this precisely what various agile approaches (scrum or XP in particular) are fighting against? Maybe that was the ambition, but I don't see this borne out in practice. If anything, Agile as most commonly practiced exacerbates this problem. Of course highly paid Agile consultants will dig their heels in and say "You're not doing Agile!", but that doesn't really change the reality for the people on the ground.
dom111 4y ago
archive.org mirror: https://web.archive.org/web/20221014021506/https://www.sledgeworx.io/code-review-handbook/ https://web.archive.org/web/20221014021506/https://www.sledg...
revskill 4y ago
This site seems uses wordpress. Currently database connection error show up.
mkl95 4y ago
Does the handbook cover load testing?
pp3355 4y ago
Link not working
- sergius 4y ago
  Seems they didn't review their code :-)
pp3355 4y ago
Link's not working
infinitezest 4y ago
One of my biggest pet peeves is when people get into the habit of leaving code reviews with only suggestions like variable name changes. It's not that I'm resistant to changing things like that but I have several colleagues that seem to only ever make suggestions like that and it seems very performative to me.
- teratron27 4y ago
  I had a colleague at a previous job who would, without fail leave reviews on only the implementation of the tests during a review. Which normally I would appreciate as testing is usually neglected but they where almost always minor nitpick style suggestions or comments (which often revealed they didn't understand the business logic / code changes being made). But as showing you proactively contributed during reviews was part of the framework for promotion they needed to show they had made suggestions rather than ask for clarification on features etc.
extasia 4y ago
I'm getting a"error establishing a dB connection"..
benniomars 4y ago
"Error establishing a database connection" I guess something must've slipped through the code review.
- alaroldai 4y ago
  http://web.archive.org/web/20221014055804/https://www.sledgeworx.io/code-review-handbook/ http://web.archive.org/web/20221014055804/https://www.sledge...
Ava4312 4y ago

118 points by Sevii 4y ago

difflens 4y ago

Nice article! I agree with all your points, they closely mirror my experience too!

kelsolaar 4y ago
Same here, there is something reassuring to see that it is quite common!

anshumankmr 4y ago

If I had a dollar for each time a merge request was reviewed by one of my peers and all they said "it looks good to me", I would have a small fortune.

m463 4y ago
Weren't you paid for creating those reviews? :)

onion2k 4y ago

The reality is that most of your comments will be reminding people to follow the style guide or to not organize their code like monkeys with type writers. If you're leaving comments about style on PRs then your tooling isn't doing a good enough job. You should have a linter and a formatter configured, everyone should be sharing the same config, and they should always be run on a prepush hook (or earlier... run them on file saves if possible). Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR. Force pushes and skipping hooks should be vigorously discouraged. That way badly formatted code shouldn't make it to the repo and your PRs can focus on more important things. Obviously this is really hard to implement on a project that's old and has thousands of linter errors. If you didn't set it up on a greenfield project right at the start you can't really complain too much though.

DangitBobby 4y ago
> Badly formatted code should make the the push fail so the dev has to fix the problem before they can open a PR I'd rather people check in their WIP than have it sitting on their machine. Let GH actions worry about whether the code is formatted correctly.
gombosg 4y ago
I think that often it's not just coding style (yes, a linter should catch indentation errors), but coding conventions/requirements to enforce an idiomatic code structure in the code base. Linters can't catch that. E.g. how to organize files, where to put business logic, how tests are structured, which libraries to use, how to handle non-blocking or concurrent code, errors, code documentation etc.
allset_ 4y ago
Style is much more than formatting. It's best practices around naming, when and how to add comments, and how to write clear and maintainable code.
- onion2k 4y ago
  I don't think I'd call any of those things "style". Those things are the code itself, not the style it's written in. They're definitely things that PR reviewers should be looking at.

doix 4y ago

It's interesting that the author puts "downloading the code" in the exhaustive section. Is it really that much work to run git checkout? I routinely checkout code in PRs if I'm not familiar with that part of the codebase. I can navigate the codebase much quicker in my editor than in GitHub. Although GitHub is actually getting there with the semantic analysis of code and finding references and stuff, sometimes I just need to open 5 files in different splits to figure out what is going on.

eastbound 4y ago
On bad projects (which happens often now that node_modules directories are often gigabytes big), IntelliJ IDEA is super-long to index, which is required before being able to Cmd+Click on functions. And we have M1. But this is an example where usability impacts the author’s judgement. For me, UX testing is part of the initial step of code reviews, no technical review is necessary if we end up deciding to implement the story another way.
- gernb 4y ago
  Well, if the code reviewers had correctly disallowed adding all those deps to node_modules then you wouldn't be in this situation. :P I'm only half joking. :P
noam_k 4y ago
I think another factor here is if the reviewer is working on the same code base and the project has a complex environment to set up (e.g., a library compiled with custom flags), then `git stash` may not be enough to checkout the branch and run tests. (I am unfortunately guilty of creating some projects like that myself...) Docker can help with these cases, but not all projects are docker-oriented. I personally like running the code myself when reviewing, it's just not always practical.
- datavirtue 4y ago
  Yeah, reality: no one is going to download code and try to run it. It would take them all day to do code reviews. You are supposed to go live with your buggy shit soon, that's what the business values. Once it's live it gets tossed over the wall and the maintenance process begins where lower paid workers tinker with it until they have the skills to code-and-bail also.

allset_ 4y ago

I think I disagree with the emphasis on asking questions. Not because questions shouldn't be asked, but because they should be asked much earlier in the process than the PR stage. Every feature should at a minimum have a brief doc for the requirements and high level design. If you haven't read that and are reviewing the PR for anything other than style, you need to take a step back and go read that doc first. In my opinion, the levels are: 1. Barely-existent review: The PR author is the SME, I'm only reviewing and LGTMing this because peer review is needed to submit the change. I might notice a typo or something along those lines, and will call out if they're being lazy and skipping tests. 2. Functionality-focused change: Does the code meet the objective and do the tests adequately cover the change? 3. In-depth review: Does the code address the problem in an efficient way that is not likely to cause the team future pain?

imiric 4y ago

It seems like a decent guide, though like others mentioned, you should download and run the code most of the time, except for trivial changes. Also, I'd like to push back on this: > Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. I disagree; it adds a lot to your bottom line. In fact, reading and reviewing code is one of the main tasks of a developer, perhaps more important than writing code. Reviewing code gives you an opportunity to get familiar with other parts of the code base, understand why a change or fix is done, and collaborate with others to make an overall better improvement to the product. It gets frustrating when egos get involved, and people take criticism too personally, or point out improvements for the sake of demonstrating their knowledge. In best working environments, code is judged without a bias or relation to who produced it, but as an inert output of the collective team. I would actually like to see a feature in code review tools to submit merge requests anonymously, just so that the author isn't a prevalent factor while reviewing. :) I usually dedicate a few hours, maybe even half a day, to reviews. Definitely don't look at it as a chore to put off, but as a crucial part of your responsibilities as a developer.

joshka 4y ago
If you read that statement as "when judged on your output by those in control of your salary" does it make more sense which bottom line is in play here?
- imiric 4y ago
  It depends. If your employer is a company that values good software development practices, then they wouldn't judge you based on the LOCs you produce, but on the overall value you provide to the team, product and company. If they're not, then most of what we're discussing here is not very relevant, since they likely don't follow other good practices either. If you're in that situation, then you have a few options: if you value the product and company, promote these best practices so that the company can attract and keep good developers, which ultimately benefits the product. If you don't, or they won't listen, or you don't have the authority to change things, then you can probably forget about following good practices, and just keep working based on the metrics they do value. Or, I'd strongly suggest, just find a better working environment. :)

marcyb5st 4y ago

I also blame the tooling a bit. After using Critique [1] internally at Google all the other tools/implementations/UIs feel very confusing. And I'm not speaking with lack of integration with other tools like coverage, linters, spell checks, error-prone constructs analysis, ... but simply the UI. Additionally within Google (or at least the teams I was in) really tries to chunk changes in small pieces. For instance, adding a feature always entails something like: 1 CL to introduce the flag that will enable/ disable the feature, at least 1 CL that actually adds the logic and tests, 1 that enables the flag in the deployment. Not sure if that's due to the monorepo or more of a culture thing, but that MO is not that common in my personal experience [1] https://abseil.io/resources/swe-book/html/ch19.html https://abseil.io/resources/swe-book/html/ch19.html

codeapprove 4y ago
I agree that the tooling is very underrated! It’s strange that programmers spend so much effort choosing their code writing environment (IDE, etc) but just take what they’re given when it comes to code reviewing (mostly GitHub PRs) If you’re missing Critique, allow me to shamelessly plug CodeApprove: https://codeapprove.com https://codeapprove.com Just like Critique, it’s a code review tool for power users that really lets you focus on resolving every discussion.
- marcyb5st 4y ago
  Thanks! Will definitely have a look!
- rob74 4y ago
  Well, you can decide more or less by yourself what tools you use for writing code, but the code review tool is something everyone needs to agree upon (and then convince management to change), so of course it has more inertia. I'd love to use something else than Jira for issue tracking too, but if the whole company uses it...
  - omarelbie 4y ago
    What would you go with instead of Jira? At a small startup here so there's still hope

airocker 4y ago

I tried to get code reviews to be done after merge. Nobody wanted to do it.

quickthrower2 4y ago
Optimistic rollups I guess
jbn 4y ago
and if you do do it (speaking from experience) people get offended that someone be looking at their code (as if existence in source control guaranteed absence of bugs...). It is all the more surprising since simple inspection of never-reviewed code usually has a lot of low-hanging fruit in terms of bugs.

azangru 4y ago

A handbook? Isn't it more like a hand article? The pdf from 2018 is 6 pages long.

azangru 4y ago

> Most developers have a lot of features to add and bugs to write, time spent reviewing other people’s work does not add a lot to your bottom line. Does not add a lot to your bottom line? This is the mindset of isolated individual contributors rather than members of a single team that work on a given feature of the same product. Isn't this precisely what various agile approaches (scrum or XP in particular) are fighting against?

yakshaving_jgt 4y ago
> This is the mindset of isolated individual contributors rather than members of a single team that work on a given feature of the same product. From experience, I think this is the more common scenario, unfortunately. > Isn't this precisely what various agile approaches (scrum or XP in particular) are fighting against? Maybe that was the ambition, but I don't see this borne out in practice. If anything, Agile as most commonly practiced exacerbates this problem. Of course highly paid Agile consultants will dig their heels in and say "You're not doing Agile!", but that doesn't really change the reality for the people on the ground.

dom111 4y ago

archive.org mirror: https://web.archive.org/web/20221014021506/https://www.sledgeworx.io/code-review-handbook/ https://web.archive.org/web/20221014021506/https://www.sledg...

revskill 4y ago

This site seems uses wordpress. Currently database connection error show up.

mkl95 4y ago

Does the handbook cover load testing?

pp3355 4y ago

Link not working

sergius 4y ago
Seems they didn't review their code :-)

pp3355 4y ago

Link's not working

infinitezest 4y ago

One of my biggest pet peeves is when people get into the habit of leaving code reviews with only suggestions like variable name changes. It's not that I'm resistant to changing things like that but I have several colleagues that seem to only ever make suggestions like that and it seems very performative to me.

teratron27 4y ago
I had a colleague at a previous job who would, without fail leave reviews on only the implementation of the tests during a review. Which normally I would appreciate as testing is usually neglected but they where almost always minor nitpick style suggestions or comments (which often revealed they didn't understand the business logic / code changes being made). But as showing you proactively contributed during reviews was part of the framework for promotion they needed to show they had made suggestions rather than ask for clarification on features etc.

extasia 4y ago

I'm getting a"error establishing a dB connection"..

benniomars 4y ago

"Error establishing a database connection" I guess something must've slipped through the code review.

alaroldai 4y ago
http://web.archive.org/web/20221014055804/https://www.sledgeworx.io/code-review-handbook/ http://web.archive.org/web/20221014055804/https://www.sledge...

Ava4312 4y ago