Ask HN: Do you do code review? | Hacker News Reader

Ask HN: Do you do code review?

43 pointsjadeydi48 comments

Do you do Code Review? Any good tool or suggestion?

100% would recommend doing code reviews.

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.

Seconded. Code reviews obviously helps for catching the small bugs/inefficiencies that the submitter hasn't thought of, but a surprising side-effect of having mandatory code reviews is that it actually helps everyone learn about the system as a whole, together. There's no better way to learn than to see someone (sometimes even yourself!) try something, hear that it's not correct, and see the fix. The failing and recovering steps are sometimes more helpful than just seeing "this is how we do it"-comments.

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.

This is a great list!

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.

Using a linter goes a long way for this. Maybe make linking part of the check in process.

Of course we do. Code reviews help us in multiple ways:

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

Could you elaborate on your experience with pair programming?

In every professional team that I've been on, nearly every change has gone through code review. Skipping code review is only done with a very good reason, like if there's a time-sensitive change that you need to make and nobody is around to review it. Another reason to maybe skip code review is for prototype projects or internal tools where low-quality code is ok.

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.

I work for a tiny 2-man project and we need to save all the time we can, so we don't code review. We actually have an unofficial policy: all code that is directly user-facing (web design for example) is handled by me and all backend stuff is handled by the other guy.

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

With all due respect, your process is likely short sighted. In the long term, you would probably benefit from code reviews. I find code review very effective even in very small teams.

short sighted, but that's sometimes the right way to prioritize. sounds like kenning is in prototyping mode, and could be intentionally prioritizing time to first test, acknowledging that s/he might have to rebuild things later. in the early stages of a startup, speed to insight can easily be more important than long term coding efficiency. so short sighted thinking can make sense.

I agree with “sometimes”. However from Kennings post it sounded like “always” and that code review is of no value.

> and that code review is of no value.

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.

Thanks for your clarification.

Yes i do whenever possible - that is, if the team i work with as a freelancer already does or is willing to follow my advice to do it. That is - disturbingly - not always the case.

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!

Suggestions? 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.

We don't do code reviews because we pair program 100% of the time. Our belief is that pairing is like embedding a code review in the process.

We discussed that option, but decided against it because code reviews also inform the larger team what was done for any particular feature/story/bug, as well as become an opportunity to teach new members of the team how different parts of the codebase are put together. So for us, they are worth the time even when we are confident in the quality of any specific PR.

I can appreciate this philosophy but I don't think it's right for everyone. On the one hand, it's nice to have a safety net for times when pairing turns into one person programming while the other is spacing out. On the other hand, if you require a review on top of pairing then you're now requiring 3 people to get this chunk of code out. So there's definitely a balance and you have to decide what's right depending on your team and situation.

I would literally rather be unemployed than have to interact with someone for the full 8 hours every day.

We also believe pair programming is "continuous code-review", but we have found that's not enough on its own. We swap pairs frequently--every week or two to ensure that knowledge isn't siloed in pairs. We also have frequent discussions in varying-sized groups prior to introducing a new service or significant function as well as retros for larger completed units after being in production for a while.

Yes, fairly regularly. We do "agile" at my 9-5, and we have at least one code review per user story that we complete, usually when the story is about 2/3 of the way done.

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.

We use Crucible [1] for reviews and Jira [2] for task management. Each review links to a Jira task (or handful of related Jira tasks). On each review, we ideally:

- 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

[1] https://www.atlassian.com/software/crucible

[2] https://www.atlassian.com/software/jira

I’m at one of the big 5, so everything goes through code review now, but since we’re a small team, we do monitoring ourselves, and as a result we have a lot of ways to bypass the process in case we need to get a high priority hot fix out.

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

Why bypass the code review for high priority fixes? That's when you need it the most.

Depending on what the issue is you might want a quick fix right away which can sometimes be hacky and then put in a long-term fix that is code reviewed by others to make sure it’s the right solution.

The big example is config changes during ongoing incidents. You may need to flip a flag or update a config right now, but waiting for review, even if it takes just a few minutes, can blow slas.

There's ways to handle that somewhat though.

Also at a big 5. We've found that skipping reviews causes more problems than it solves in emergency situations. It's really tempting to bypass the process because you're sure you can fix it fast, but this mindset results in more missteps in aggregate.

If a couple minutes really makes that much of a difference you should probably page multiple people in from the start of an issue.

Generally speaking, your un-reviewed change isn't a "fix", but something to stop the bleeding. It might move traffic out of a problematic area, or disable an experiment that's leading to unusual metrics.

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.

This is absolutely the wrong thing to do. When mitigating issues on call anything that goes live bypassing canary has to receive much more scrutiny, not less. It is best not to do such things, but if you have to do them, review the change extra hard.

See my other comment to a sibling expressing a similar sentiment. Rollbacks and drains may require submissions to change configurations, but aren't bypassing canary, they're specifically reverting to known good systems.

At a medium sized company. Everything gets code reviewed by one or two teams. Only one or two approvals are required. Stash/bitbucket is used and we leverage the Default Reviewers feature. The process works very well, is integrated with our ticketing system (Jira) and build system (Bamboo). We are nice in the code reviews but thorough and a little pedantic about style.

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.

I suspect this may not be too well received, but I've thus far managed to avoid regular formal reviews, and I'd advise others to think twice before imposing them. They seem like a big step towards treating programmers like cogs in a machine rather than competent individuals, and for me that's a direction I don't want to be headed.

Have you tried doing them?

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?

Yes, I've encountered them. It seems really hard to do them in a way that doesn't touch heavily on style. To the extent that the preferred solution to trying to shift the focus back to deeper issues is to go one step further and mandate auto-formatting tools. So individuality-of-style has been dismissed out of hand.

Can they be helpful? In some cases, perhaps yes. But they're definitely pushing a collective-ownership, try-to-smooth-away-individual-differences agenda.

It is much easier to read code that is written consistently, and it's much easier to contribute to code when a style guide is available. This isn't "pushing an agenda". And what's wrong with collective ownership?

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.

I find that good programmers will voluntarily request reviews whether or not they are required by process, and bad programmers will attempt to minimize them even when required by process. Having a policy is a good way to weed out the bad people, and won't affect the good people.

I might agree with you in the sense that sometimes there's a worthwhile discussion to be had and of course in such cases it's good to talk to someone who's opinion you trust.

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

And moreover, I send my gnarliest PRs to the most vicious reviewer on the team, just because 3 months from now finding out what’s wrong is exponentially more difficult. I want to get it right the first time around, and I want other folks to be able to make changes anywhere in the codebase, even in the gnarly parts.

We have mandatory code reviews for everything, which encourages small, bite-size commits and frequent merges. Majority of the changes goes through with just one other person taking a look.

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.

TBH I favor atomicity over small size. So we get a few gnarly PRs a week. That’s also why we use Reviewable instead of GitHub reviews

Seconding Geoff, I find code review to be one of the best forms of mentorship possible for a junior to mid engineer.

A workflow using feature branches off of master that show changes visually using github, bitbucket, and similar tools works well.

Yes. No code gets into the master branch without a code review unless it's a well defined critical fix during an outage.

Code reviews keep me honest. I know I’m often sloppy and I wan’t to rush things. Code reviews keep me in check. It’s when team work in software development manifests at its best. You need good trust among peers though or it can be destructive. Code reviews are not about looking the smartest or putting others down.

How do you motivate people to actually review the code? Not just accept it.

By requiring reviews for merging into master in GitHub and postulating that unless something is in master it does not exist in the product.

What you propose is perfectly compatible with people merely blindly clicking 'Accept" instead of actually reviewing it, which is the problem of the parent poster.

Explain the value. Then spot check and separately talk to those who blindly click accept, and if they continue to do so, get new engineers.

I would 100% recommend pair programming. Its baked right in!

Reply on news.ycombinator.com