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.
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.
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."
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.
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  for reviews and Jira  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
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).
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.
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.
I would 100% recommend pair programming. Its baked right in!