http://stackoverflow.com/questions/310813/how-do-you-perform-code-reviews
How are code reviews performed on your development team?
I've been a developer for several years now in several different companies and I have noticed that there isn't a consistent approach to performing code reviews.
At my current company, code reviews are non-existent, which has led to a significant decrease in the quality of the code. At previous jobs, code reviews ranged from just making sure coding standards were enforced to nazi-like line by line reviews that took days to complete.
So I'm wondering what its like for everyone else out there. And in particular, what tools do you use to perform the reviews? And do you find that code reviews help rather than add to the length of time needed for a given project?
I'm all for code reviews as I believe they are an effective way of spotting problems early in the development cycle and they can help novice programmers become good programmers, good programmers become great programmers, great programmers become excellent programmers and so on and so forth.
Up until now, I've only seen code reviews done manually via comments in Word and Excel documents....but just recently, I saw an ad on this site for a product called Code Collaborator and it appears to be a much better way to do code reviews.... One nice feature I noticed is that it can force code reviews to be performed before code can be committed...
Has anyone on here ever used this product or a similar one? And if so, have you found it makes the process of doing code reviews easier?
Teams around Microsoft use a variety of different CR methods. Some teams are very formal, others more informal. To my knowledge, all teams do one form of code review or another
Our team tends toward a more informal code review style. New chunks of code, large or complex changes first go through a face to face walk through. Several of us gather in a conference room and the author walks everyone through the changes. The point is to ensure everyone understands why the author did things they way they did.
Before a walk through, participants are expected to have read any specs and other docs and gone through the code on their own. The walk through is more for answering questions and understanding than providing constructive feedback. As the dev manger, I really discourage comments on style in this meeting: our teams have well defined and documented coding standards that folks are expected to follow. We're not draconian about it, but we'll cancel a walk through if myself or one of the leads fells the code isn't ready for a walk through due to style issues.
After the walk through, other feedback is handled via email. We don't formally track the feedback - email provides sufficient record.
We use developer branches for each person so folks actually check in changes into their branch before code review. This is easier than trying to keep changes out of our source code control system before it is reviewed.
Team members send changes based on code review feedback to other team members in what we call a "BBAPCK". This is a package of changes that can be viewed, diffed or temporarily applied to a local copy of the code base. Usually the lead will approve the final BBAPCK for integration into the parent code branch.
Incremental changes for bug fixes or small improvements are simply handled via mail. Or a quick "over the shoulder" code review.
In your question, you use the term 'force'. I suggest that if you have to force your team members to do anything by imposing tools and processes on them, then you have team culture, discipline, or leadership problems you need to fix.
For our team, the dev general has discretion on which changes he will make from CR feedback. But, the leads and senior devs can require some things. We don't have any rules on this - team culture drives this. We almost never have a disagreement about CR feedback. We have a culture of providing good constructive feedback. CR feedback is conditioned helpful; anything that makes the code better is goodness.
This light weight process works well for us: We have enough process to catch bugs in CRs, but its light weight enough for us to move quickly and be efficient.
Our biggest challenge is scheduling time for CRS... even though we do a lot by email, some CRs take significant time. This is hard to predict and schedule. But, we prioritize CRs highly - only rarely skipping them to meet schedule. Though that does happen.
I've worked at companies where code reviews absolutely sucked. We spent way more time arguing about style issues such as what to name variables, or where curly braces go that more substantial topics. Of course, consistent style is important, but focusing on correctness, reliability, performance, efficiency and maintainability is way more important than style.
We keep the code in the team style by encouraging people to do it: If somebody checks in a little "ugly" code, the lead simply fixes it.
I have one project where the code base (about 90K lines of code) has three distinct styles. This is due to a few things: 1) I had a team member (no longer with us) that just refused to follow the team style. We have not had the time to go re-format all his code. 2) Some of our code follows another teams style because it is based on their code.
My view on style is that it should be consistent in each module, and related modules, but its not too problematic if it varies a bit. This is true for one of my teams. The XPERF tools team is very different - it is very important to them that their code has a very consistent style for all modules. This works great for them.
As a dev manger; I find it important to be flexible here and let teams (and some individuals) have the freedom to set their own coding guidelines and standards. Its important to developers and at the end of they day; devs that are not bent out of shape about coding guidelines enjoy their work better and get more work done.
Now, some things are not left up to teams or devs. In windows we have many practices and requirements that we follow with exacting discipline. For example, all product code must be fully localization. Many practices around security and reliability are always adhered to. Nobody gripes about these because we all know how important they are.
We check for these things using some very sophisticated tools - many of which now ship in Visual Studio 2008. Others are checked for in our code reviews.
I encourage everyone to work with their team to find a set of CR practices and habits that work well for the team. If you are a manger, I encourage you to take a 'light hand' with pushing what you feel may be the right thing over the objections of your team. Of course, you must exercise your judgment and bring your experience to the team; but fighting over CR practices and style issues can ruin a good team - I've seen it happen.
At the end of the day, focus on two things:
- Shipping great code that delivers what your customers want; is reliable, performs well and meets its other requirements.
- Maintaining a fun and rewarding work environment for your team. After all, coding is fun after all. If its not, then your management team is horked. Effective code review practices can be a very positive force for both of these goals.
Answered by Foredecker