RSS Feed

Latest Entries

Wednesday
Sep092009

Step #4 - Start doing code reviews – Seriously

As one team lead to another, as someone who has actually done this with a real team, I can tell you that this is one of the few effective techniques to maintain a high quality bar in your code base, and to keep your people learning. It is also one of the beest ways to stay in touch with the codebase even though you are a team lead, and have much less time for real coding.

Whatever you do, don’t listen to others or even your own inner voice telling you that it’s a waste of time.

Code reviews are nothing short of a magical process of growing people, code quality and confidence at the same time, and it has changed the way we work at Typemock immensely.

What's a code review?

One developer looking at another developer's source code - thoroughly.

What do you look for in a code review?

  • Logical bugs
  • stupid bugs
  • Unreadable code
  • Ugly code that should be refactored

What is really the purpose of code reviews?

  • Besides catching bugs really early on, you mean?
  • Keep code at a much higher quality
  • share knowledge of new feature, requirements and code between people in the team (the reviewer learns about code they did not write)
  • Raise skill of both reviewer and review-ee ("oh I'll show you a much more elegant way than doing this ugly for-loop - see?", "That looks great - what do you call this pattern?")
  • Let the team know that you, the team lead, care - about them, about the code, about quality

Who should do the code review?

  • At first – Just you.

Everything has to start somewhere - and as a team lead, I expect you to know good code, and to recognize bad code (here's a good book about this subject or even better - a whole bunch)

  • Other team members (when you feel comfortable with it)

How and when should you do a code review?

There are two phases to this. The first one is the one where you bring the team up to par with what you consider quality code - You will be the only one doing code reviews - and no code is checked in before passing your code review. seriously. This is serious business and you should stand your ground. I cannot begin to tell you how much good this will do to the team - but you must stand your ground and give this a try.

  • Sit down with the team, or explain this at the end of the next daily standup -

From now on, every code being checked in has to pass a code review first.

That's right - every check in will be reviewed by you personally.

  • Wait for the next time someone is about to check in their code
  • Sit with the, and find the changed code. You can use the "show changes" tools from  the various source control tools (aka sourceSafe, SVN, VSTS etc.).
  • Go through each file and read the code. Do this even if the file checked in is not code – a text file, a solution file – it’s amazing how many gems you can find)

many times when reviewing you’ll find classes that are not in use anymore, or fully commented out, code that is duplicated and forgotten to be removed and many more “gems”.

  • Ask important questions about every little thing that bothers you:
    • "Why are those comments there?"
    • "Can we make this more readable?"
    • "This needs refactoring to smaller methods" and so on.
    • “How could you make this code more readable”
    • “What do you think is wrong with this piece of code"?”
    • “What would be a better name for this method?”

 

  • As you comment on the code have the developer put a special “REVIEW” or “REFACTOR” comment on that piece code which is easy to find later.
    • do not hesitate to say it if you feel that they are not paying attention or that they are not writing down those comments.
  • When you finish the review ask them to make any changes and show you the code again before checking it in.
  • Do a quick pass on the changes that were made (you can pass on this step after a couple of days if you feel comfortable enough)

Train the team to review its own code

I’ve said this previously, but I’ll mention this again:

Your most important role in the team is to act as a coach – to teach them to solve their own problems.

To that effect, it’s important that you not become the bottleneck for the team. The goal here is to make sure every checked in code is being reviews, without the team slowing down to wait for you every time. IN order to achieve that, have a third person sit with you and the review-ee (that’s 3 people on the same machine) and write down a list of what you are looking for in the code. That list will later be used to coach others on code reviews.

Have that third person change between reviews so everyone gets to learn.

The next step

When you’ve code code reviews for say- a week or two, it’s time to let the team start doing its own reviews. 

In the next standup meeting, declare that from now on, everyone can review each other’s code, with your supervision. From now on you will be the third person sitting behind two team members as they review each other’s code. As a team member reviews the code, sit quietly and only speak when they prepare to move on to a different file. If you have something to say, that’s when you can say it:

  • “What about naming conventions in this file? have you looked at those?”
  • “I’m still seeing something in this code that I think you missed – care to try and find it?”
  • “Why did you choose to apply this refactoring instead of X?”
  • “Why would you name it X?”

and so on.

typically, within a couple of days you should feel comfortable with them reviewing each other’s code, and free you up for other things.

 

What did we end up with?

When all is said and done , 2 or 3 weeks later, you should be able to say:

  • My team knows how to write better code today than they did last month
  • My team learned a new skill this month
  • My team can raise the quality but still be fully independent in doing so
  • I feel that I know exactly how out code looks
  • I know my team members better than I did last month

Those are things that you should be very proud to say. These are the hope of any team lead – that their coaching actually makes a difference.

You might still want to reviews code every now and again, and make sure the bar is still set to your standards. If it lowers again, you can repeat the exercise.

You should also have a better understanding of the strengths and weaknesses of each team member in regards to coding, sharing information, pairing and overall attention to detail. These will help you a lot when it comes time to grow people. I’ll touch on this in a future post.

« Is face to face pairing better? | Main | How to find hidden problems in your project before it’s too late »

Reader Comments (9)

Timely post...I just had a recruiter ask if we do code reviews at my current job. We do, but nothing formalized. Thanks for putting a framework out there to go by.

September 9, 2009 | Unregistered CommenterJeff

Great post, I never looked at it that way before. I will try to make this more frequent then it currently is.

September 9, 2009 | Unregistered CommenterCory Mathews

As you comment on the code have the developer put a special “REVIEW” or “REFACTOR” comment on that piece code which is easy to find later.

Could you explain this a bit? Why would you want to find this reviewed code later on?

September 11, 2009 | Unregistered CommenterSteven

Steven: The changes are not made as the review is going on - so after the review is finished - the dev goes through the comments and refactors according to what has been decided in the review.

September 11, 2009 | Unregistered CommenterRoy Osherove

Nicely written, but I would be a bit cautious about always taking a "From now on, every code being checked in has to pass a code review first" approach. That might work with some teams, but not all. Another option is to start a bit more slowly, perhaps with just checkins to the stable branch, or checkins to certain files that the team has identified as the highest risk. More tips available here: http://smartbear.com/docs/BestPracticesForPeerCodeReview.pdf. Additional tips on managing the social effects of introducing code review available here: http://blog.smartbear.com/the_smartbear_blog/2009/03/tips-for-developers-social-effects-of-code-review-part-3.html

September 11, 2009 | Unregistered CommenterGregg Sporar

Well said!
This is exactly what I wish to do! I coach myself to be able to coach others. Code review is very important and I realize its need day by day.
And the only way to make it happen in proper way is what is mentioned here. I recognize that everyday as if you speak out my mind.
The only issue that it will take time! and might take some long time depending on my skills and knowledge as well as my teams skills and knowledge. So as you mentioned in Art Of Unit Testing, there are questions and resistors to such unit testing. Same thing for code review. If not fully resisting they would just say "Ok, but what you want to do is just too much!" And at the end we are limited in time for delivery and so on.
What do you think about that? Standing my ground isn't an easy thing! specially if you are going to be blamed for delivery delay!!

September 15, 2009 | Unregistered CommenterMuhammad Mosa

Roy - By every commit do you mean to the trunk? I encourage the team to commit often but use a personal branch for this. Even then, I would expect multiple merges to the trunk each day. That makes code reviewing pretty much a full time job for me for a while!

September 19, 2009 | Unregistered CommenterNeil

Roy - Some of the code we have is huge and written over the last 5 years. It all needs reviewing, but this would take days(weeks even) and would totally depress the developers concerned. They want to freeze it and move on by rewriting it rather than refactoring it. Rewriting means a very big project which would be great given unlimited resources and no business pressure for additional functionality. How would you propose tackling this kind of issue with large amounts of legacy code?

(btw, one of our team is looking forward very much to your course in the UK next week.)

September 19, 2009 | Unregistered CommenterNeil

This post is as I have written it. This exact practice is what I do as team lead every day so I can claim that it works and that it pays off.
After some time, team members don't wan't to checin their code until they get it just right.
Then you delegate them to inspect each others code, and also (VERY IMPORTANT) check your code. You should be very careful here and do your best to admit your mistakes and motivate others do continue to criticize your code.

September 19, 2009 | Unregistered CommenterVukoje

PostPost a New Comment

Enter your information below to add a new comment.

My response is on my own website »
Author Email (optional):
Author URL (optional):
Post:
 
Some HTML allowed: <a href="" title=""> <abbr title=""> <acronym title=""> <b> <blockquote cite=""> <code> <em> <i> <strike> <strong>