In addition to the generalized rules below, each team should consider having their own set of guidelines for code reviews. Things like "how fast code reviews progress", "who has domain knowledge that's relevant" and how you handle risk may be worth spelling out for your group.
Too many points to summarize, but:
- ask for code review in a way that respects others' time
- distinguish opinion and fact when reviewing
- avoid picking nits (e.g. update style guides instead of going line by line)
- communicate expectations about timeliness of reviews
- discuss architecture/overall design before a code review
To which I'd add: you should communicate expectations about who and how many people go on reviews. When you get a code review, you should also feel free to say "ask so-and-so to review".
Having juniors review senior commits:
- familiarizes the junior dev with the code base, especially with best practices
- gives them a stake in the team
- prepares them to maintain the project
- gives the team a new set of eyes that may challenge their assumptions
I appreciate the point about not adding tangentially related work to a PR "while someone is at it".
Paying attention to the distinction between opinion and fact a distinction is important. I'd add a distinction between personal opinion and team opinion. If your team always puts certain functionality in the X class, then just saying "please move this to the X class" is enough. Personal opinions merit the more involved discussion suggested here. Similarly, one HN commenter described their code review style as almost exclusively consisting of questions.
Results of a large scale of "lightweight" code review at Cisco. Some key points:
- code reviews should cover no more than 200-400 LOC
- reviewers should spent between 5-90 minutes on a review
- having authors annotate their code substantially improved results (without making reviewers complacent)
Argues that managers need to spend time on making sure that their code review process is healthy, and is adequately training their developers. The manager should rarely comment on the first level technical issues, but should address how the process works.
Imagine a system for editing and reviewing code where:
Every branch of every repo gets its own sandboxed directory. Your revision history in each branch, including uncommitted stuff, is persisted, as are build artifacts. When you switch contexts, each project is just as you left it.
Within your editor, you can pull up a global view of all your branches, your outstanding pull requests, and the pull requests you’re assigned to review. It’s one keystroke to view the summary for a pull, and one more to start editing any of its files right there under your cursor.
Code review happens entirely within the editor. You’re fed a series of diffs: one keystroke to approve, one keystroke to start editing. Dive in, make your changes, leave comments for the author, push, and move on.
Hard not to think this will be table stakes for development environments in 10 years. HN Comments (very noisy) have some suggestions other environments can do this, but it's unclear to me whether those comments are accurate.
Part of a series on Iron, Jane Street's second generation code review tool
- Code review tools should be incremental (if you look at a patch twice, you should, by default, only review each file once).
- Discusses how to handle cases where a diff leads to conflicts.
Describes how Iron assigns a level of required scrutiny to each patch, based on the area of the code base it touches. The reviewer is informed of how much scrutiny the code requires, and Iron requires that multiple reviewers approve code that has higher scrutiny levels.
Exhaustive list of code review resources, more than almost anyone should read.