Code reviews are an important part of any project's health. They are able to find and correct code defects before making it into the product and thus spare everyone the pain of having to find them and take them out. They are also relatively cheap. Assuming your team wants to implement code reviews, it is important to lay out the expectations clearly. Code reviews can be contentious without some guidelines. What follows are the rights and responsibilities as I see them in the code review process.
Participants:
Reviewer - Person (or persons) reviewing the code. It is their job to read the code in question and provide commentary on it.
Reviewee - Person whose code is being reviewed. They are responsible for responding to the review and making necessary corrections.
Must Fix Issues:
A reviewer may comment on many aspects of the code under review. Some of the comments will require the reviewee to change his code. Others will be merely recommendations. Must fix issues are:
- Bugs - The code doesn't do what it was intended to do. It will crash, leak memory, act erroneously, etc.
- Potential maintenance issues - The code is not broken, but is written in such a way that it will be hard to maintain. Examples might be magic numbers, poorly named variables, lack of indirection, lack of appropriate comments, etc.
- Coding standard violations - If the group has a coding standard, it must be followed. Deviations from it must be fixed when pointed out.
Recommendations:
Other items are merely recommendations. The reviewer can comment on them, but the comments are only advisory. The reviewee is under no obligation to fix them. These include:
- Architectural recommendations - The reviewer thinks there is a better way to accomplish the goal. Seriously consider changing these, but the reviewee can say no if he disagrees.
- Style issues - The reviewer wouldn't have done it that way. Fascinating.
Code Ownership:
In my teams there is no ownership of code. Some people touch certain pieces of code most often and may even have written the code initially. That doesn't give them special rights to the code. The person changing the code now does not need to get the initial writer's permission to make a change. He would be a fool not to consult the author or current maintainer because they will have insights that will help make the fix easier/better, but he is under no obligation to act upon their advice.
Good Blog as always, Steve.
ReplyDelete"Style issues - The reviewer wouldn't have done it that way. Fascinating."
Laughed at this initially, assuming the 'Fascinating' was sarcasm, simply because two programmers will rarely come up with the same solution for the same problem. This begs the question how many different ways do you want to solve a given problem in your application? While coding standards are well and good, they rarely deal with nitty gritty level of detail you hit in a code review, and can still fail to answer this question. I use code preview as well as code review, i.e. a discussion with how you intend to write the code prior to commencing coding, but still hit this problem on occasion.
I suppose you can always say the code meets requirements, and mark it and it's similar counterparts as candidates for a future refactor, but it seems less than satisfactory Any thoughts on this?
Style might not be worth fighting for when time is pressing but if during a code review the reviewers suggest a different approach and the reviewee chooses to ignore this I see serious trouble in your project. You probably chose you reviewers based on their experience no? I have seen a lot of bad designs (I hate the word architecture) make it to production because some 'architect' wanna-be decided to do his or her own thing. Luckily these people usually get fired soon after.
ReplyDelete