During the last 15 years I worked for a few different organisations of different sizes.
Some of them had more or less formal code reviews as part of their software development process. In my current job for example we have a very rigid code review process that says that every bit of code should get reviewed. But why?
I guess code reviews are considered as a way to improve code quality. It's believed to have a learning effect for less experienced developers as they have to review other's code and their code is reviewed and they should get valuable feedback on their code.
That's all nice in theory but my experience is different.
In a real world project it's unlikely that you have enough understanding of other parts of the code to do really useful code reviews. (I know collective code ownership is something that we should try to aspire for but again - I have still to see that.)
So most code reviews end up with advises like: add some JavaDoc there, don't forget to cleanup imports and format the code (Yes. I know what save actions are for. Thank you!)
All of this stuff (and much more) can be done easily by PMD, FindBugs and CheckStyle. Done properly by an continuous integration server it's automatic and easy. Additionally it's much easier to accept feedback like this from an automatic system than it's to accept from the JavaDoc Nazi next door.
The saved time can be invested into stuff that really improves the code. You could start to do pair programming for at least the important modules. That's really something that creates collective code ownership and makes it possible to have more than one person that really know what's going on inside these core parts. Additionally less experienced developer can learn from the other's and the experienced developers learn how the other's think and code.
But I'm still trying to get this into the heads of my coworkers and managers.
Subscribe to:
Post Comments (Atom)
Posts
Agreed: anything that can be automated with tools like PMD, etc. should be automated. It's a waste of time to have developers point out those issues.
ReplyDeleteYour comment about "it's unlikely that you have enough understanding of other parts of the code to do really useful code reviews" is the crux of the issue.
Even if, however, I don't understand the code well enough to ask "why did you do it this way?" I can still ask: "is this the functionality that the code is supposed to deliver?"
For example, one technique is to just review the unit tests (assuming that your team is using unit testing). By asking about boundary conditions and assumptions made by the code, you can help uncover gaps in the implementation. No automated tool can spot bugs that are caused by an incorrect interpretation of the requirements (e.g. "no, the code is supposed to divide that value by 10 in that situation, not by 20.").
Pair programming, as you point out, is a great way to get to collective code ownership (especially if you mix up the pairs on a regular basis). But if the team won't go for pair programming, code review can be an important step in the right direction.
I also think that doing code reviews is better than nothing. But I think that in a lot of cases they are done wrong or at least they could be improved.
ReplyDeleteIn my current job we are doing formal "after the fact" reviews. When the code is already committed to the SVN someone has to review it. This might happen sooner or later. Maybe much later depending on the current workload.
In a former job we had no formal reviews but depending on how important the stuff was we did "pair reviews" before committing to the SVN or merging into the trunk.
I think that pair reviews are the way to go if pair programming is not an option.
Sitting in front of one PC and reviewing the code together with the original author really makes a lot of sense. Especially if you have a huge chunk of code to review.