Quick fixes to your code review workflow

https://consulting.drmaciver.com/code-review-quick-fixes/

by hyperpape • 4 years ago

113 44 4 years ago

consulting.drmaciver.com

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• 4 years 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• 4 years ago

Totally. Looking at the diff and asking "But why did I make this change? And how would I justify it?" is very helpful.

notemaker• 4 years 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.

vinnymac2• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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• 4 years 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.

fishywang3• 4 years 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.

mook• 4 years ago

You can add to the set of refs that git pulls (via remote.origin.refs); so for me it's `git checkout pr/123/head` (or /merge) instead, without creating a local branch that you need to delete later.

nurgasemetey2• 4 years ago

Also there is github cli for checking out PR

     $ gh pr checkout {<number> | <url> | <branch>}

hpb42• 4 years ago

You can also configure git to fetch the GitHub PR branches[0]. Add this line to your .git/config:

    fetch = +refs/pull/*/head:refs/remotes/upstream/pr/*

This has to be done in each repository you have checked out locally, but maybe it is possible to also have this done globally. But I never tried to do it.

[0] http://tiborsimko.org/github-local-handling-of-pull-requests...

sa46• 4 years ago

IntelliJ's integration with GitHub PRs is nice. The one thing I miss is you can't what changed if you switch from the PR diff view to the normal code view which you need for code navigation and refactoring.

Vinnl• 4 years 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".

vinnymac2• 4 years 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

weeksie3• 4 years 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• 4 years ago

I detest pair programming

why?

weeksie3• 4 years 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.

fishywang3• 4 years 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• 4 years ago

What's the difference?

fishywang3• 4 years 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.

hipnoizz• 4 years ago

I've spent probably close to a decade using Gerrit with various sized teams, and in the past few months I had an opportunity to work with Gitlab (so the MR/PR model). I've found that I definitely prefer Gerrit (and this feeling is shared by my colleagues). I think it boils down to following reasons.

Gerrit is a CR tool only (OK, it also hosts you repositories) and I've found that its UI allows me to focus on the code being reviewed much better than Gitlab. There are multiple maybe small things (navigation patterns, keyboard shortcuts, they way the information is provided) but all together make a tangible difference, one of reason being that reviewing someone other code is not an easy one and any obstacle will make it more mundane and lead to very shallow reviews.

I've found that a commit-based review process (although it is not enforced, i.e. a developer can push whatever number of commits and then ask for a review/assign reviewers) is much (much) better than the PR/MR model. I think it nudges developers into a) more though-out commits, b) smaller commits, c) pushing early and getting early feedback. Probably the fact that we are working with a bit unnecessarily complex branching model now makes things worse, but even without that I think that an effort to push changes (especially smaller ones/small fixes/small improvements) is (much) smaller in Gerrit.

There is one oddity with Gerrit, namely so-called 'Change-Id' (the way Gerrit tracks new revisions) - nothing unmanagable, but it probably will make you trip a few times at the beginning. And likely you will need to learn Git a little better (in general you will need amend/rebase to apply changes after your colleagues commented on them). If you are using any of JetBrains IDEs there is a very good plug-in, which also allows to easily fetch changes you are asked to review. All in all in the past decade we were able to on-board all new hires without much pain.

I still like many other bells and whistles provided by Gitlab (and likely Github), especially build pipelines. I would like to find some time to try to 'integrate' Gerrit with Gitlab - Gerrit can easily push changes to other Git repository, so I can imagine it would be possible to plug into build pipelines etc. etc. Yeah... so many things to do, so little time ;-)

Edit: One more thing - it is mentioned by fishywang below/around - I've just read his comment and facepalmed about myself why I didn't listed it as well - tracking comments/discussion/changes/revisions/diffs - this may be the biggest advantage of Gerrit - really, maybe I don't know how to use Gitlab properly, but this aspect there is really weak.

cjcenizal• 4 years 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• 4 years ago

Mutation testing is basically a method of automating this.

jimmont• 4 years 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.

nurgasemetey2• 4 years 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• 4 years ago

Nice job touching on backward compatibility. It’s underrated.

Quick fixes to your code review workflow

Related Stories