The most effective way I've found to do code reviews is to create a pull request for the code you would like to have reviewed. Send it out to someone or a few people that you would like to take a look, get them to approve or reject it, and then take a look at their comments and see what you can do to address them.
Some guidelines for code reviews:
1. Build each other up. The purpose of a code review is to make sure the code is good, but also to help the team grow together. If growing isn't the focus, then code reviews can quickly become harmful.
2. Get code reviewed early, often, and in small chunks. No one wants to review a 1,000 line change spanning several sub-systems, and this is the quickest way to get people to just blindly approve or to discourage them from taking the time to review. Smaller changes make it easier to justify taking 10 minutes to look over the code and make some good, constructive suggestions. It also makes it easier for the person writing the code to implement the suggestions.
3. Don't take it personally. It can be easy to let our ego get in the way when reading someone's comment about our code - after all, we might be proud of it after having spent 45 minutes on the 10 lines that someone just said "could be cleaned up"! Keep in mind that code reviews are about growing and learning, building better systems, etc.
4. The review is about the code, not the coder.
5. Honesty is key. Don't be a jerk, but don't avoid making suggestions just because you're worried it might offend someone. That just makes the review pointless!
6. Be willing spend time with the reviewer if you wrote the code, and vice-versa if you reviewed the code, to go over the suggestions made. Sometimes a comment isn't enough to adequately explain something.
I could probably write more suggestions, but I think these cover the basics.
As for tools, I just use PRs. Create one, either add certain people as reviewers or put a link to the PR somewhere that you can ask for reviewers, and then await their approval/rejection/comments. Once you have approval from the reviewers, merge it. From there you can start to build out different processes/rules around it as you see fit, but it doesn't have to be complicated and doesn't require anything fancy.
I would recommend doing them more async as opposed to scheduling "code review meetings" however. These tend to be more wasteful and can introduce a lot more stress.
I'd like to suggest one if you don't mind
7. Review your own code before handing it over. Time is wasted when the submitter left out obvious issues such as typos, missing comments, unnecessary complexity, debug code, etc.
Consider the reviewer's time precious because context switching is expensive and she/he might not get back to it as quick as you'd like to.
Implementing clear guidelines and linting would help.
Regarding the big diffs: sometimes it's inevitable because you're doing a big formatting of the code, or you're just doing a no-op refactoring introducing new primitives. If you do so, separate commits with one being the refactoring with no functional changes, and one being the actual changes. Makes review much easier.
- You have 2 sets of eyes on every piece of code committed to master (You may argue pair programming does the same, but in our experience, pair programming requires a very different mindset, esp. hands-off pair programming)
- Multiple people know what changes have gone into master recently. We have an alias (code-review@) which gets org wide code reviews and is added by default and every eng has an option of tracking/commenting on code reviews.
- For cross team efforts, it's helpful since a team which might have context on a change can see the change before the actual commit. "Coding to interfaces" is great, but seeing under the hood offers a different view as well
For the actual process, we use ReviewBoard (https://www.reviewboard.org/). It has decent integration with command line so you can post reviews from the command line itself. We also have a git hook set up which looks for "R=<someuser>" in each code commit, and rejects the commit otherwise. The hook is (intentionally) super simplistic so you can get past it using something like "R=Self", but with a strong engineering culture, we see this happening less and less with non-minor code commits.
Code review is not only a good way to spot bugs, it spreads knowledge and context throughout the team and makes sure everyone is consistent about both high-level and low-level coding practices. This is especially important when two people are writing code that will integrate with each other; normally each will look over the other's code. It also ensures that if something breaks, there are at least two people who might know how to fix it. Every software engineering context is different, but it seems like in most cases, it's irresponsible to ship code to users that has only been seen by one person.
GitHub, Phabricator, and Gerrit are all good code review tools. I would recommend both using a tool like this and having a habit of talking through code changes in person.
My previous company worked the same way though, and it was a real company with funding and customers. It was another tiny startup (essentially 4 person engineering team) and the work was cleanly divided: one person did the entire iphone/android app (react native), one person did the website, one person did the backend and one person did the analytics. For an established company this would be a nightmare -- we had tons of bugs in production, usually reported by our users. However, having a really clean separation of duties made it extremely efficient in terms of management, because everyone knew that if they wanted something done on the mobile app they just asked the guy who did the whole thing, he knew the codebase thoroughly and could fix or add the change almost immediately, then would work on the next task on his trello board (which almost nobody else even looked at). We essentially had no management (CEO was constantly gone looking for more funding) so this organizational structure was a huge boon, and allowed us to push out huge features in 1-2 weeks. I think that if we had a full time QA person this really could have worked well, and by "well" i mean "we could have done the job of a ~15 person engineering and QA team with only five people."
I don't agree with this at all. Code review is obviously a huge benefit once you have enough money to pay more than one engineer per gigantic slice of the codebase.
I'd say code review is also great if everyone is working on the same thing, such as a library. In that case I'd do code review if there were only two people working on the project. But when one guy is basically just doing react boilerplate and implementing designs, it's not worth the time to check every commit they make.
I only worked with gitlab abd github pull/merge request code review tooling and found it ok.
There’s Gerrit, but i had no chance to work with it yet.
Obviously always depends on how they need to be done. When in doubt, just sit next to each other and look at the part of code and changes for the current task your working on that is about to be merged. Not always a special tool necessary and if you don’t know which one to use better just start than let the tool choice get in the way. That being said I’m not against tools!
As with any process or meeting, define what the scope is!
Is it about coding style? Which are your guidelines for that one then? Is it about getting a second look if the story is implemented in a good, efficient way?
Is it to verify definition of done rules are met? About having (good) tests?
It’s probably not (counterexample i just had) to discuss the actual requirements defined by the story the code should fulfill... that should have happened at another time, probably with other persons, like the Product Owner.
How often? I think it’s best to do it with each merge request for the code it contains, and maybe in defined tinespans for the whole project.
How big is your team? Do you all work in the same physical location, or are you remote?
We don't use any fancy tools. We just go back through the commit logs for the branch and diff from before we started to present. For any major refactoring, the lead on that user story will walk the rest of the team through the architectural changes.
We recently moved to Visual Studio Team Services, which has a few more niceties as far as code review goes. I'm looking forward to digging in, but in no way do I think that tools like that are necessary to perform and derive value from code reviews.
- Add 3-4 people as reviewers
- Reviewers ask questions, make suggestions
- Author follows up to these questions (online or offline), and if they make code changes they link to the diff in a comment
- Close reviews only if 2+ people have finished reviewing
- It's okay to close reviews that not everyone has finished reviewing
When I was doing a small startup, we had a small phabricator installation which I loved using. It’s very engineer-focused in a lot of its tooling and features, but has an integrated task management systems and wikis, and tools for pre-commit reviews, or post commit reviews/audits, and I found it much easier to run and maintain than github enterprise or the Atlassian suite (and it’s free).
There's ways to handle that somewhat though.
If a couple minutes really makes that much of a difference you should probably page multiple people in from the start of an issue.
In other words, its a rollback, not a fix-forward. (and I'll note that where I work, these changes still need to be reviewed soon after they're submitted). I agree that all fixes should be reviewed before submission.
The only reason I can picture not doing code review is if there is only one developer. It is an excellent way for everyone to learn, reduce bugs, reduce spaghetti code, force you to write tests, etc.
That's different from mandatory code review, which almost inevitably will end up covering style and process considerations.
It's certainly a filter, but depending on your perspective I'd be a little cautious about "won't affect the good people".
But we also specify (and highly encourage) that anyone trying to land a more invasive or complex change should require multiple review approvals. The most demanding "please pile on, we want eyes on this" request I remember was set to require 4 separate approvals.
Other users have already described healthy code review processes that improve code quality while helping all participants to grow individually and as a team.
How does review treat programmers like cogs in a machine?
Can they be helpful? In some cases, perhaps yes. But they're definitely pushing a collective-ownership, try-to-smooth-away-individual-differences agenda.
Code review shouldn't involve bikeshedding about whether braces should go on the next line or not. It should be about design decisions, clarity of expression, algorithm complexity, etc.