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.
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!
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".
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?
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.
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.
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.
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.
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?
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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.
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".
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.
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.
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.
Azure DevOps can also show the changes between different pushes: called “updates”. We tried GitHub for some time, but found the lack of this feature a no-go. GitHub is seriously broken in this regard: they will send an e-mail that should show the difference since the update, but clicking this link will show a 404 as the old HEAD was GC’ed. See also https://github.com/isaacs/github/issues/999 and related issues.
> How in Github's PR model can you see the diff between the 2 states of a PR?
This is super easy with SmartGit and it works with any Git host.
The Branches panel lists both my local branches and all remote branches. I double-click the remote branch and it offers to create a local branch for it.
Then I can compare any two commits by simply clicking one of them in the Graph (log) panel and Ctrl+click the other one I want to compare. Whenever two commits are selected, the Files panel lists the files that changed between the two.
Click any of those files and the Changes panel shows the changes between the two commits. Double-click the file to open a more full-featured compare tool such as Meld.
The Branches panel has a couple of other great features: a list of your stashes, and a Recyclable Commits checkbox. When you turn on any of those, their commits appear in the Graph panel just like any other commit.
So if you've really messed up, instead of having to trawl through the reflog and checkout a commit you think might have the code you need, you can just click around and view any reflog commit or compare any two commits. Same for stashes. Instead of all these special cases, every kind of commit is unified into one consistent model.
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.
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.
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.
