How do you approach code reviews?

I think it's difficult to give advice on code review in isolation of the rest of your development process - code review is but one small piece in a good workflow. I'll describe what has worked best for me in the past.

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.

First rule: trust your colleagues. If you think you’re a good developer, chances are your colleagues are good as well (otherwise they wouldn’t be hired in the same company as you; they wouldn’t even be in your same team).

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 first step is to have a good coder who can organize their commits into simple self-contained small steps that can be reviewed individually. Then your job gets 10000% easier. If I giant mess of a commit ends up assigned to me I just send it back or stall. If you want my attention you have to help me help you.

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".

I would recommend discussing with your team.

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.

I'm building a better interface for code review for teams on GitHub, one that helps you reach consensus quickly and decisively.

If anyone here is interested please email me at sam@habosa.com