on May 26th, 2010Code Reviews: Good, A Complete Waste of Time or a Necessary Evil?
Eclipse developers are in the middle of the ‘End Game‘. Half way through the release candidates and getting ready for another major release. All the API is in place, the features are ‘complete’ and we are working through the last of the major bugs.
One thing that changes dramatically during the end-game is that each code change requires a review. As we get closer to the release, the number of eyes that must review the code increases. The theory is that with more eyes on the code, the less likely we are to break something. It also forces us (the committers) to explain our changes and convince others that the change is really needed. Often just explaining your code is enough to make you see the flaws in it. Finally, the reviews give us confidence in our changes.
But for a development team that normally does not perform code reviews (at least it’s not part of the mandated development process) the 2 month end-game really mixes things up.
I’m personally enjoying the code reviews. Of course it gives me more confidence in the code I’ve written (or in some cases I walk away with my tail between my legs), but the reviews are also teaching me a lot. We all have our own way of doing things. I have my own coding style; and looking at how others solve problems (and getting feedback on my solutions) is one of the greatest benefits of working on open source. I view software development as an ongoing learning process, and there is no better way to learn than to critique others and have other critique you.
There are two common methods for reviewing code:
- Review then commit: The code is reviewed before it is committed to the repository
- Commit then review: The code is reviewed after it has been committed (and removed or changed if needed)
So my questions to you are:
- Do you find value in code reviews? or are they just a wast of time?
- Have you been involved in open source or close source code reviews (or both)?
- Which method of code review have you tried? Which one works better?
Related posts:
- OSGi and Equinox book complete!
- Summer of Code — Congratulations students
- Java One – Good looking apps with RAP
- ESE 2010 is over. It’s time for the November DemoCamps
- Type Inference at Design Time via Eclipse
for me code review is essential.
I mean it is not only about early bug discovery. for me it is more a collaboration mechanism, the instance where you share your experience with others and you learn from others.
a must for any kind of project with more than 1 developer.
I’m the development lead where I work, and for me, code review helps with two things. First the same errors will not be repeated. If you don’t specify that a developer did something wrong, they will make the same mistake again and again. You might think it is easier just to fix the problem yourself, but in the long run you will save yourself a lot of work by educating the developers. Secondly it makes it easier to evolve the code in the future. If a part of the system is full of WTF’s a bug fix will either require a big cleanup (with the risk of adding more bugs), or require adding another WTF. Making sure the code is good from the start makes life much easier in the future.
I think that cartoon code review is pretty close to the truth. The other phrase you hear is “HTF did this *ever* work?”
Like Christian says, if you are a lead and have the responsibility to guide and educate developers, then code review is a very powerful tool that will help bring developers along. I rarely found any bugs of consequence in code reviews, more likely it would be an assessment of the maintainability and elimination of persistent conceptual flaws.
IMHO code review are indispensable when you build complex software in a team. It helps in the following areas:
- spot bugs early (the earlier, the cheaper)
- educate the developers (and the lead)
- spread knowledge about the codebase (technical depts, capabilities, etc.)
- practice team collaboration
The main gotchas are in the human interaction. If you have people in your team, that love their job and are proud craftsmen, reviewing is a pretty sensitive task on a human level. Their code is their baby, they’re mostly not neutral about it but proud or ashame
So I think that it’s pretty essential to review, discuss and communicate with tact. Its pretty important to motivate rather than frustrate. If you reach this goal, reviews are a big gain for all participatns (devs, product owner, the product in it’s whole).
* Do you find value in code reviews? or are they just a waste of time?
They definitely not a waste of time, because they avoid bugs while they are created. Being forced to do reviews also helps to keep your source tidy, so it becomes even possible to be reviewed.
Noone wants to present a total mess to his coworker.
Beside the educational part, I think its a wonderful opportunity to pull up low-skillers to a higher level by letting them review code from those high-skillers. “Clean Code” gives even not so experienced developers a great chance to learn what makes code good.
* Have you been involved in open source or close source code reviews (or both)?
We announced it as obligatory to review before committing within our team.
No commit without a review. From the very beginning.
Its hard and it needs people who watch this process.
* Which method of code review have you tried? Which one works better?
I haven’t tried the commit-then-review process yet. I think, if you work remotely it might be more easy to do so. But I also think it might put the build in danger.
I think you’ve forgotten another possibility:
3) Commit, push to review area, review, then push to master
Having a distributed version control gives you the best of both worlds – just see http://egit.eclipse.org/r/ for an example of the Gerrit code review instance that permits many people to review, comment upon, and then check for changes subsequently, as part of the EGit and JGit projects.
By the way, Ian, you have a misplaced close bold/strong tag after the ‘com’it in option 1.
While manual code reviews by peers is a good thing, it also should be supplemented with automated review tools such as FindBugs, PMD, CheckStyle, and others. There are many possible bugs that we as programmers take for granted saying “that will never happen”…or in “HTF
…did that happen.” . Code views are just the start, not the end. They should also be balanced by Unit Tests, and code coverage analysis reports as well.
* Do you find value in code reviews? or are they just a wast of time?
Depends on who wrote the code.
* Have you been involved in open source or close source code reviews (or both)?
Both.
* Which method of code review have you tried? Which one works better?
Both. They both have their place. If a rookie wrote some code then it is good to review it before a commit. During a project it is always good to check all code you get through the version control system and finally it is good to review code when you merge a project branch into a main branch. All these reviews are executed with different levels of detail.
* Do you find value in code reviews? or are they just a wast of time?
Depends on who wrote the code.
* Have you been involved in open source or close source code reviews (or both)?
Both.
* Which method of code review have you tried? Which one works better?
Both. They both have their place. If a rookie wrote some code then it is good to review it before a commit. During a project it is always good to check all code you get through the version control system and finally it is good to review code when you merge a project branch into a main branch. All these reviews are executed with different levels of detail.
.
As Alex mentioned, the EGit/JGit projects are having a lot of success in using Gerrit as its code review system. The only missing piece for me is Mylyn integration to sweeten the deal. Not every patch has to be reviewed, but the system serves as a nice proxy in notifying everyone that something is going in with the potential of review before being pushed to master.
Full disclosure: I work for a company that sells a peer code review tool. So we’re biased, but we’ve also talked to many software developers about code review. With respect to reviewing before or after commit, there is no one correct answer. As Alex Blewitt pointed out, the capabilities of your version control system need to be considered, among other things. More on that here: http://blog.smartbear.com/the_smartbear_blog/2009/04/code-review-before-or-after-checkin.html.
André Dietisheim brought up an important point about the human interaction aspect. More on that topic here: http://blog.smartbear.com/the_smartbear_blog/2008/12/addressing-obje.html
Unfortunately, from my experience, code review is something that is always talked about being done, but seldom happens. When it does happen, it is nothing more than looking at a method or two, and making sure there are comments.
Code reviews are good, but sometimes there is only one person who really knows the code being modified. If the reviewers are not familiar with the code, it is hard for them to do more than just point out “obvious” mistakes.
This isn’t a reason to skip the reviews, but it does mean that when you are close to release time, you need to be extra careful and perhaps not make the change at all. Just because the code was reviewed, doesn’t mean there isn’t a hidden problem nobody is seeing.
The cartoon made my day. LoL!
At my day job, no code goes into the repository without code review (before or after commit).
We use Atlassian’s Crucible, which is an awesome tool (the best by far in their suite, IMO).
More than spotting bugs, I see code reviews as a great way of ensuring code readability. If it cannot be understood by a reviewer, not even the author will understand the code a few months later. Other wins are sharing of knowledge (both ways) and establishing a common style of development.