11 ms·

Quick fixes to your code review workflow

113 points by hyperpape 4y ago

thinkingkong 4y ago
One of the biggest improvements Ive made and rolled out was to do my own review first. Scan through all the code from the reviewers perspective and try and see where adding a comment, renaming something, or explaining a decision might be beneficial. It makes life way easier for everyone.
- franzb 4y ago
  Absolutely! This works even better when doing the self-review with the same tool (typically, GitHub review system) as the one a real reviewer would use. Just looking at one's code in a different environment (different font, colors, etc.) helps switching from a "programmer's mind" to a "reviewer's mind".
  - AndrewDucker 4y ago
    Totally. Looking at the diff and asking "But why did I make this change? And how would I justify it?" is very helpful.
    - XorNot 4y ago
      Except this is the problem with looking at patches in Github, as opposed to as source code. Github shows you such an utterly minimal view of the overall flow of the code that it's impossible in a lot of cases to tell what the intent was or why - you don't even have function-level context for it. Compare to if you simply checked out main and the PR, and ran a meld across both directories - changes would be highlighted, but now you have to read the code in context.
  - notemaker 4y ago
    I, too, need to review my code in Gitlab/Github/Gerrit in order to catch errors - somehow they're much more prominent there (like you said, I suppose it's due to adopting a "reviewer's mindset") rather than just looking at `git diff`. But if possible I would like to review my work in the terminal as a part of my workflow, not needing to context switch to the browser. Has anyone found a good solution to that? FWIW, using tmux & nvim.
    - aidos 4y ago
      I always review before committing with git diff in the terminal but I just find that once I create the PR I’ll spot something else when reviewing there. Not sure what it is but I think it’s just taking that moment to read through from a different perspective (might even be that a different ux gives that perspective). I don’t especially love the review workflow on GitHub - but I can definitely catch bugs there.
- vinnymac 4y ago
  This is actually a requirement on my team. If someone does not do a self review, I leave a comment explaining that I won't be reviewing the work until the decisions that were made are explained. This helps understand the justifications for the change, and prevents unnecessary feedback loops to an incredibly high degree.
  - b3morales 4y ago
    In my opinion this is the purpose of a commit message. Header: short description of the change; body: details, including justification for the change. But I agree, it's also good when people preemptively make comments on their own commits in the code review interface. Question for you though: do you have a size limit on this requirement? I.e., changes that are small enough don't need to do this?
- pojzon 4y ago
  This often saves me a lot of time. Not only on finding simple issues but also more complex ones. Self-Review should be advertised more as a clean-code practice in the industry.
- UglyToad 4y ago
  I think this is about 98% of the value I get from code review, just looking at the diff in the GitHub view generally results in me spotting all the bugs and stylistic issues that would be raised in review. This makes waiting days for the actual approval all the more frustrating!
  - onion2k 4y ago
    This makes waiting days for the actual approval all the more frustrating! In your next stand up raise the fact you're waiting for a review as a blocker, and watch as reviews suddenly start getting done much more quickly.
- dimal 4y ago
  1000x yes. When I finish a significant PR, I almost always wait until the next day to put it up. When you’ve spent 3-4 days grinding away at a problem, the urge is to just put it up and get rid of it. But when I sleep on it and look at the code from someone else’s perspective the next day, I always find a ton of easy fixes that I didn’t see at 5pm the previous day. It makes the review so much less painful for everyone.
- elpakal 4y ago
  100%. I’ve found that taking a step back, going on a hike, riding my bike, walking the dog or just doing anything not coding related for a little bit before I go back and review my code really helps me catch things I miss when I don’t take a break. Even better after a night’s sleep. Distance lends perspective and all that.
pmcollins 4y ago
My brain is a very bad virtual machine and it's very bad at parsing diffs (even GitHub ones), so I like to check out the code under review locally, browse it and see what my IDE says about it (thank you JetBrains), run it, and run the tests while stepping through the code. Makes a big difference.
- codeflo 4y ago
  That’s an underappreciated technique, and I’m not sure why. Someone once remarked (verbally) in the context of a code review that they would need to run the code to fully understand it, to which I replied “Why don’t you?” The look on their face was priceless, first confusion, then enlightenment. There’s no law or invisible wall that prevents you from leaving that web-based diff viewer.
- indymike 4y ago
  One of the best moves our team ever made was moving to code review means actually checking out and running the code in our IDE. So many little improvements happened: logs got better, runtime-messages got better. Since we were reviewing in the IDE, running the profiler was a snap, and dropping a breakpoint in a diff lising let the reviewer inspect state right at the point of change. Another benefit: it was a lot easier to fix bugs found in review than just punt to the original developer... so the reviews became a lot less painful.
- stumpf 4y ago
  A previous coworker taught me to do this :finger-guns: I use VSCode with the GitHub pull requests extension now, and it's such a better experience than attempting to review directly on github.com. My reviews are not just quicker but also higher quality since I can easily step through code changes directly in the IDE. Great advice.
- fishywang 4y ago
  I have this script: $ cat ~/bin/git-pr #!/bin/sh if [ $# -lt 1 ]; then exit -1 fi git fetch origin refs/pull/$1/head:pr/$1 So I can run `git pr <number>` to get `pr/<number>` branch created locally that I can checkout to.
Vinnl 4y ago
These are good tips. One more thing, if we're talking GitHub specifically: the labels of the statuses you can give your review are somewhat misleading, in that their actual effects don't quite match the labels. So: - "Request changes": this isn't so much a request, as it is "prevent this person from merging this until I specifically review it again". It's fairly harsh. - "Comment": effectively, "require another review before it can be merged, not necessarily by me". - "Approve": the submitter can merge it at their prerogative. It is very common for me to hit "approve" even though I've requested changes, as they're often > so trivial that you don’t think it needs a second review afterwards or even totally reasonable for someone to push back against or not implement. Sometimes I'll have seen a non-trivial bug, in which case I'll set it to "comment". I practically never use "request changes".
vinnymac 4y ago
I rarely see this mentioned, but one thing that has made a huge improvement to my day to day, is enabling scheduled reminders for code reviews. I have it configured with email and Slack right now, and never miss a review. Rather than having to keep tabs on the state of reviews, or synchronously discuss a review request, I can just wait until I get a review request in my notification tray. It's been the most seamless experience I have found so far. I am not sure if Gitlab or Bitbucket offer similar functionality, but you can configure it here on Github. https://github.com/settings/reminders https://github.com/settings/reminders
weeksie 4y ago
I was happy to see he mentioned "pair" reviewing. I detest pair programming but pair reviewing is excellent. Whenever the time and culture of a place permits, I do everything I can to walk through my pull requests with one or two people. It's such a high value activity, especially when it comes to teams with a good mix of junior and senior people.
- rpastuszak 4y ago
  > I detest pair programming why?
  - weeksie 4y ago
    I don't like it for myself because I don't work well that way. I don't like working at companies that use pair programming because most of the code I've seen come out of pairing sessions is not very good. I realize that's likely to start a religious war in certain areas but I've been doing this for 20 years and that's been my experience so far. I think pairing sessions can be great for teaching or for getting some needed distance from a problem, but as a mode of production I haven't enjoyed my experiences with it.
    - BurningFrog 4y ago
      One rarely mentioned thing about pair programming is that it's a learned skill. If you put two arbitrary programmers in front of a computer and ask them to pair program, you'll probably end up with two annoyed people, and not much code. I was fortunate to join a team of experienced pairers who showed how it's done, and after ~3 weeks of ramping up, I was fully into it.
      - weeksie 4y ago
        The failure mode I usually see on experienced pairing teams is overly complex code. There's a certain flavor that I can usually pick out and be pretty sure that a couple of Pivots or Carbon5 or whatever pair shop has been working in a codebase. I wish I could be more specific about what sets me off, but I've called it blind enough times to think that there's a real effect. Not that single coders don't produce weird code, just that I've not experienced a situation where pairing has been a win—with the BIG exception of doing code reviews.
fishywang 4y ago
A better fix is to abandon the whole "pull request" model used by github (and copied by a lot of others, like gitlab, etc.), and use a proper code review system (gerrit, etc.) instead. But of course that's not a "quick" fix.
- yodsanklai 4y ago
  What's the difference?
  - fishywang 4y ago
    A lot. Here are some examples: Say you are the reviewer, you gave some feedback for the author to address. The author says they addressed them and asked for your review again. How do you review that? Of course you can just review the whole change again, but if it's a huge change and you only asked a few small parts to be changed, it would be hard to make sure the rest are still the same. How in Github's PR model can you see the diff between the 2 states of a PR? The only way is for the author to push a separated, "fixup" commit to the review branch, so you can just view the diff of that commit. But that creates those "fixup" commits that's not useful in the final merge, so you will want to use squash merge in the end, and using squash merge means that the final commit message is totally up to the people clicking the merge button and others have no control over that. In gerrit, commit messages are part of the code that you can review and leave feedback, a change is always a single commit, and there's builtin feature to show diff between the 2 states of a change. Another example is when you have a change (B) depending on another pending change (A), and there are some feedback on change A you need to address, and it would be a whole mess on how you also update change B to make sure that changes of A do not show up in the diff of B.
    - g4zj 4y ago
      Say you are the reviewer, you gave some feedback for the author to address. The author says they addressed them and asked for your review again. How do you review that? I can't speak for GitHub, but GitLab allows you to easily compare previous versions (states) of a branch. Whether you push additional commits or squash them locally and force push to update the remote branch, the differences between versions are easy to compare. https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...
cjcenizal 4y ago
Sometimes for particularly complex changes, I like to comment out lines of logic and see if any tests break. It’s a fast and easy way to identify if the test coverage is adequate. Code coverage report generators are a good way to automate this, but not all projects use them and sometimes just because logic branches are covered by a test doesn’t mean the variations in behavior created by those branches have been covered. Tests that pass after I’ve deleted some code flushes these problems into the open.
- raziel2p 4y ago
  Mutation testing is basically a method of automating this.
jimmont 4y ago
A tremendously helpful article. Possibly one of the best HN shares I've seen related to software. Most code reviews I've seen professionally over the past decade are almost entirely stylistic one-way brittle dialogs over text of small to large pull/change-requests. When in reality these could, should, recommended to be (as the article shares) side-by-side discussions for dialog to improve the product being contributed to. It's really an effective way to ensure that someone else can pick up the work and go if the other gets hit by a bus, and in my own experience, when I lead with an explanatory walk-through exposes bugs (to myself) along the way--that even the reviewer often doesn't note. Thanks for sharing, software development practice is several decades behind... other practices, and content like this help to improve it.
nurgasemetey 4y ago
I quickly improved my PRs and sped up code reviewing by doing following when I create PRs - make a diagram if is possible - make a summary of changes like why this file changed and decision behind this - make a note about backward compatibility - anticipate which questions can be asked and answer them upfront
- jiux 4y ago
  Nice job touching on backward compatibility. It’s underrated.

113 points by hyperpape 4y ago

One of the biggest improvements Ive made and rolled out was to do my own review first. Scan through all the code from the reviewers perspective and try and see where adding a comment, renaming something, or explaining a decision might be beneficial. It makes life way easier for everyone.

franzb 4y ago
Absolutely! This works even better when doing the self-review with the same tool (typically, GitHub review system) as the one a real reviewer would use. Just looking at one's code in a different environment (different font, colors, etc.) helps switching from a "programmer's mind" to a "reviewer's mind".
- AndrewDucker 4y ago
  Totally. Looking at the diff and asking "But why did I make this change? And how would I justify it?" is very helpful.
  - XorNot 4y ago
    Except this is the problem with looking at patches in Github, as opposed to as source code. Github shows you such an utterly minimal view of the overall flow of the code that it's impossible in a lot of cases to tell what the intent was or why - you don't even have function-level context for it. Compare to if you simply checked out main and the PR, and ran a meld across both directories - changes would be highlighted, but now you have to read the code in context.
- notemaker 4y ago
  I, too, need to review my code in Gitlab/Github/Gerrit in order to catch errors - somehow they're much more prominent there (like you said, I suppose it's due to adopting a "reviewer's mindset") rather than just looking at `git diff`. But if possible I would like to review my work in the terminal as a part of my workflow, not needing to context switch to the browser. Has anyone found a good solution to that? FWIW, using tmux & nvim.
  - aidos 4y ago
    I always review before committing with git diff in the terminal but I just find that once I create the PR I’ll spot something else when reviewing there. Not sure what it is but I think it’s just taking that moment to read through from a different perspective (might even be that a different ux gives that perspective). I don’t especially love the review workflow on GitHub - but I can definitely catch bugs there.
vinnymac 4y ago
This is actually a requirement on my team. If someone does not do a self review, I leave a comment explaining that I won't be reviewing the work until the decisions that were made are explained. This helps understand the justifications for the change, and prevents unnecessary feedback loops to an incredibly high degree.
- b3morales 4y ago
  In my opinion this is the purpose of a commit message. Header: short description of the change; body: details, including justification for the change. But I agree, it's also good when people preemptively make comments on their own commits in the code review interface. Question for you though: do you have a size limit on this requirement? I.e., changes that are small enough don't need to do this?
pojzon 4y ago
This often saves me a lot of time. Not only on finding simple issues but also more complex ones. Self-Review should be advertised more as a clean-code practice in the industry.
UglyToad 4y ago
I think this is about 98% of the value I get from code review, just looking at the diff in the GitHub view generally results in me spotting all the bugs and stylistic issues that would be raised in review. This makes waiting days for the actual approval all the more frustrating!
- onion2k 4y ago
  This makes waiting days for the actual approval all the more frustrating! In your next stand up raise the fact you're waiting for a review as a blocker, and watch as reviews suddenly start getting done much more quickly.
dimal 4y ago
1000x yes. When I finish a significant PR, I almost always wait until the next day to put it up. When you’ve spent 3-4 days grinding away at a problem, the urge is to just put it up and get rid of it. But when I sleep on it and look at the code from someone else’s perspective the next day, I always find a ton of easy fixes that I didn’t see at 5pm the previous day. It makes the review so much less painful for everyone.
elpakal 4y ago
100%. I’ve found that taking a step back, going on a hike, riding my bike, walking the dog or just doing anything not coding related for a little bit before I go back and review my code really helps me catch things I miss when I don’t take a break. Even better after a night’s sleep. Distance lends perspective and all that.

pmcollins 4y ago

My brain is a very bad virtual machine and it's very bad at parsing diffs (even GitHub ones), so I like to check out the code under review locally, browse it and see what my IDE says about it (thank you JetBrains), run it, and run the tests while stepping through the code. Makes a big difference.

codeflo 4y ago
That’s an underappreciated technique, and I’m not sure why. Someone once remarked (verbally) in the context of a code review that they would need to run the code to fully understand it, to which I replied “Why don’t you?” The look on their face was priceless, first confusion, then enlightenment. There’s no law or invisible wall that prevents you from leaving that web-based diff viewer.
indymike 4y ago
One of the best moves our team ever made was moving to code review means actually checking out and running the code in our IDE. So many little improvements happened: logs got better, runtime-messages got better. Since we were reviewing in the IDE, running the profiler was a snap, and dropping a breakpoint in a diff lising let the reviewer inspect state right at the point of change. Another benefit: it was a lot easier to fix bugs found in review than just punt to the original developer... so the reviews became a lot less painful.
stumpf 4y ago
A previous coworker taught me to do this :finger-guns: I use VSCode with the GitHub pull requests extension now, and it's such a better experience than attempting to review directly on github.com. My reviews are not just quicker but also higher quality since I can easily step through code changes directly in the IDE. Great advice.
fishywang 4y ago
I have this script: $ cat ~/bin/git-pr #!/bin/sh if [ $# -lt 1 ]; then exit -1 fi git fetch origin refs/pull/$1/head:pr/$1 So I can run `git pr <number>` to get `pr/<number>` branch created locally that I can checkout to.

Vinnl 4y ago

These are good tips. One more thing, if we're talking GitHub specifically: the labels of the statuses you can give your review are somewhat misleading, in that their actual effects don't quite match the labels. So: - "Request changes": this isn't so much a request, as it is "prevent this person from merging this until I specifically review it again". It's fairly harsh. - "Comment": effectively, "require another review before it can be merged, not necessarily by me". - "Approve": the submitter can merge it at their prerogative. It is very common for me to hit "approve" even though I've requested changes, as they're often > so trivial that you don’t think it needs a second review afterwards or even totally reasonable for someone to push back against or not implement. Sometimes I'll have seen a non-trivial bug, in which case I'll set it to "comment". I practically never use "request changes".

vinnymac 4y ago

I rarely see this mentioned, but one thing that has made a huge improvement to my day to day, is enabling scheduled reminders for code reviews. I have it configured with email and Slack right now, and never miss a review. Rather than having to keep tabs on the state of reviews, or synchronously discuss a review request, I can just wait until I get a review request in my notification tray. It's been the most seamless experience I have found so far. I am not sure if Gitlab or Bitbucket offer similar functionality, but you can configure it here on Github. https://github.com/settings/reminders https://github.com/settings/reminders

weeksie 4y ago

I was happy to see he mentioned "pair" reviewing. I detest pair programming but pair reviewing is excellent. Whenever the time and culture of a place permits, I do everything I can to walk through my pull requests with one or two people. It's such a high value activity, especially when it comes to teams with a good mix of junior and senior people.

rpastuszak 4y ago
> I detest pair programming why?
- weeksie 4y ago
  I don't like it for myself because I don't work well that way. I don't like working at companies that use pair programming because most of the code I've seen come out of pairing sessions is not very good. I realize that's likely to start a religious war in certain areas but I've been doing this for 20 years and that's been my experience so far. I think pairing sessions can be great for teaching or for getting some needed distance from a problem, but as a mode of production I haven't enjoyed my experiences with it.
  - BurningFrog 4y ago
    One rarely mentioned thing about pair programming is that it's a learned skill. If you put two arbitrary programmers in front of a computer and ask them to pair program, you'll probably end up with two annoyed people, and not much code. I was fortunate to join a team of experienced pairers who showed how it's done, and after ~3 weeks of ramping up, I was fully into it.
    - weeksie 4y ago
      The failure mode I usually see on experienced pairing teams is overly complex code. There's a certain flavor that I can usually pick out and be pretty sure that a couple of Pivots or Carbon5 or whatever pair shop has been working in a codebase. I wish I could be more specific about what sets me off, but I've called it blind enough times to think that there's a real effect. Not that single coders don't produce weird code, just that I've not experienced a situation where pairing has been a win—with the BIG exception of doing code reviews.

fishywang 4y ago

A better fix is to abandon the whole "pull request" model used by github (and copied by a lot of others, like gitlab, etc.), and use a proper code review system (gerrit, etc.) instead. But of course that's not a "quick" fix.

yodsanklai 4y ago
What's the difference?
- fishywang 4y ago
  A lot. Here are some examples: Say you are the reviewer, you gave some feedback for the author to address. The author says they addressed them and asked for your review again. How do you review that? Of course you can just review the whole change again, but if it's a huge change and you only asked a few small parts to be changed, it would be hard to make sure the rest are still the same. How in Github's PR model can you see the diff between the 2 states of a PR? The only way is for the author to push a separated, "fixup" commit to the review branch, so you can just view the diff of that commit. But that creates those "fixup" commits that's not useful in the final merge, so you will want to use squash merge in the end, and using squash merge means that the final commit message is totally up to the people clicking the merge button and others have no control over that. In gerrit, commit messages are part of the code that you can review and leave feedback, a change is always a single commit, and there's builtin feature to show diff between the 2 states of a change. Another example is when you have a change (B) depending on another pending change (A), and there are some feedback on change A you need to address, and it would be a whole mess on how you also update change B to make sure that changes of A do not show up in the diff of B.
  - g4zj 4y ago
    Say you are the reviewer, you gave some feedback for the author to address. The author says they addressed them and asked for your review again. How do you review that? I can't speak for GitHub, but GitLab allows you to easily compare previous versions (states) of a branch. Whether you push additional commits or squash them locally and force push to update the remote branch, the differences between versions are easy to compare. https://docs.gitlab.com/ee/user/project/merge_requests/versions.html https://docs.gitlab.com/ee/user/project/merge_requests/versi...

cjcenizal 4y ago

Sometimes for particularly complex changes, I like to comment out lines of logic and see if any tests break. It’s a fast and easy way to identify if the test coverage is adequate. Code coverage report generators are a good way to automate this, but not all projects use them and sometimes just because logic branches are covered by a test doesn’t mean the variations in behavior created by those branches have been covered. Tests that pass after I’ve deleted some code flushes these problems into the open.

raziel2p 4y ago
Mutation testing is basically a method of automating this.

jimmont 4y ago

A tremendously helpful article. Possibly one of the best HN shares I've seen related to software. Most code reviews I've seen professionally over the past decade are almost entirely stylistic one-way brittle dialogs over text of small to large pull/change-requests. When in reality these could, should, recommended to be (as the article shares) side-by-side discussions for dialog to improve the product being contributed to. It's really an effective way to ensure that someone else can pick up the work and go if the other gets hit by a bus, and in my own experience, when I lead with an explanatory walk-through exposes bugs (to myself) along the way--that even the reviewer often doesn't note. Thanks for sharing, software development practice is several decades behind... other practices, and content like this help to improve it.

nurgasemetey 4y ago

I quickly improved my PRs and sped up code reviewing by doing following when I create PRs - make a diagram if is possible - make a summary of changes like why this file changed and decision behind this - make a note about backward compatibility - anticipate which questions can be asked and answer them upfront

jiux 4y ago
Nice job touching on backward compatibility. It’s underrated.