Back in the day when I was a full-time software developer code reviews were an important part of my life. One group I was part of us sent the whole team for serious training in doing code reviews. We took them quite seriously and I thought there were good two ways. 1) they made the code we produces much better – more reliable, more maintainable, more efficient. In fact better by any metric you want to use. 2) they made everyone on the team a better developer – we learned a lot from each other and from our discussions.
And they also made sure that we all understood each other’s projects which was often a huge help.
So how did we run code reviews?
Each review had three people at least. Someone to read the code out loud, someone to take notes on what was found, and the author of the code. More people was fine but three is the minimum. No people can’t take several roles. No the author of the code can’t take notes or read the code. Three roles – three people. Deal with it.
Everyone on the review gets the code in advance and reads it at least once before the review. No really you do. You are either concerned about quality enough to do this or you are not. At the review the reader starts reading – stopping at the end of each statement in case of comments or concerns. If a concern is raised there is a discussion. The code author explains what the code does and why it was written that way. People give pros and cons and a decision is made if the code stays the same or is changed. The person taking notes makes notes of any decisions involving a change.
There are lots of reasons that code might be changed BTW. I once wrote an absolutely elegant recursive solution to a minor problem and the code review hated it. Not that it didn’t work – it did. Not that the solution had bad performance – performance was fine. But rather that the code was “tricky” and hard to understand. Perfectly clear to me but not so much for the rest of the team. This code had to be maintained and perhaps one day modified so being clear was important. I re-wrote the code.
Changes may not be major either. It maybe a variable name change or perhaps some better comments for more internal documentation. Or it might be as major as a refactoring of repeated code into a new method. or more. The point is that changes are discussed and agreed to.
Which brings up another issue in code reviews. Ego. You really have to check your ego at the door before a code review. And I don’t just mean the author of the code being reviewed though that is important. Everyone has to remember that the code is being reviewed and not the person. This is not about being a good person or even a good developer – it is about making the code good!
No one should “pull rank” by saying “I’m the senior person and I say this” or “I know more about this than you” or anything of the sort. Cases for change must be made based on the merit of the idea not the person who pushes it. The author of the code has to avoid taking criticism personally. The reviewers have to avoid making the criticism personal. It’s about the code – never lose sight of that.
Code reviews can only go on for so long BTW. A marathon all day code review is pretty much as useless as no code review at all. People get tired. The author of the code starts to feel beat up and overwhelmed no matter how things go. Two hours is probably the maximum amount of time. They gave us a time limit when I was in training but after all these years (yeah I’m old) I forget the exact amount of time. Two hours feels right though.
Regardless set a limit based on time not on lines of code reviewed and stick to it. Code reviews can take time. Often mangers don’t want to spend the time and often developers just want to write code and let someone else test it and call it done. But for me there is nothing like a code review for when you want to ship the very best code.
Edit: karen Wells left the following in the comments:
"If you want more data on code review best practices, you can request a free book - Best Kept Secrets of Peer Code Review - at www.CodeReviewBook.com."