Ask HN: How do you approach code reviews?
For example do you look at all the changes at once, go commit by commit? Looking for more effective and efficient ways. Thanks!
My team are all quite git-savvy, all unafraid of a bit of interactive rebase to craft a nice clean history on our feature branches. This means that we can all trust that reviewing a pull request commit-by-commit, reading the commit messages and inspecting the changeset to verify that it does indeed achieve what it says on the tin, will actually be worthwhile. Having said that, we also try to keep the pull requests as small as possible, so a good proportion of them only end up being a single commit anyway. We do this in part by throwing up what we call "skeleton pull requests", where we submit an interface or a placeholder API definition with hard-coded return values so that the design/shape of the work can be reviewed before the meat of the implementation begins. If we notice that we need to change the design during the implementation, we'll usually throw up a draft pull-request as early as possible and ask reviewers specific questions about the parts we're changing to get their thoughts.
Second rule: don’t spend time in small details. This usually means: syntax, variable names, line length, test coverage. If you care enough about this, then install tools that verify this automatically when opening pull requests. Variable names: I know, this seems important, but bad variable names doesn’t necessarily mean bad code (keep reading).
Third rule: think long-term. This is, is the code under review easy to maintain? It’s not that subjective: how many files are being touched? All of them just for one feature? Smells like we need to refactor something else before working in that new feature. Are good abstractions present? This usually means you know a bit about the domain model and: no inheritance, dependency injection (I’m not talking about containers), and well defined interfaces.
Rule zero: no bugs (or at least no evident ones).
Regarding looking at the code at once or going commit by commit: none is a good way. Start examining the client code (or tests, sometimes) and go up to the core code (the one that has less dependencies). Usually the client code will tell you if the feature has a “good” architecture.
The Linux Kernel communities have a lot of good advice on both writing self-contained patches and properly reviewing things.
As an extra advice, code reviews can often feel very personal and are a sensitive topic. Always make sure to be impersonal: instead of "you did this and that, which is not correct" try to go with "the code does this and that, which is not correct", and "we should do that" instead of "you should do that".
Personally, I look at what is in the review and understand why the changes are being made. Then I look at each file that is changed. I look at the code changes from a few perspectives including complexity, security, best practice and conformity to the code base. I review any test code and any static code analysis tool output.
If anyone here is interested please email me at sam@habosa.com