peer reviews

I had a interesting discussion with some team members today - they wanted to know why I wasn't enforcing peer reviews before allowing check-ins.

Let me state four things first:
1. I really, really like peer reviews. They are good for many reasons, including code quality, transferring knowledge, cross checking and keeping developers aware that someone else will be reading their code.
2. I think that code quality is very important. Especially the structure of the code. I have posted about this before.
3. We work in an iterative process where code lines are built twice a week, so there are opportunities to change before the final build.
4. Peer reviews are still carried out - they may just not be before the initial check-in.

So now, let me explain why I don't enforce them before the initial check-in:

It's very important to get the changes to test as soon as possible.
- Irrespective of everything else, the most important thing is to get the changes to test. The sooner it's in test, the sooner you'll find if the solution is correct, let alone if the code is high enough quality. And the sooner it's available to show to the customer, the sooner they can confirm that you're on the right track, or for them to refine the goal.
- It's important to keep developers focused on delivering. We've all been in refactoringits, where you just need to refactor one more function... If you have to get it to test, then it helps to keep you on track.
- It's rare that I find critical issues in peer reviews. If I do find them, then I address them and the reason behind them.

Most developers write adequate code.
- Most professional developers write adequate code. By that, I mean that it will function correctly and not have obvious issues.
- In order to improve someones skills takes time and effort and most importantly, a drive from the student to want to learn. Jamming a bunch of standards down someones throat is not the best way to improvement.

Improvement will come over time.
- Hopefully your developers are improving their skills. This is something that tends to happen. Even if it's just experience with the language or it's features, they are improving. And a lesson learnt the hard way tends to stick.

Current knowledge
- You and your team have a certain amount of knowledge right now. This will change over time and you will find that what you thought was very important right now, turns out to be less so. So, you can't be too focused on certain issues.

Understand your team members.
- If you know your team mates' strengths and weaknesses, then you know what to look out for. You'll know that Jim* is great with c#, so he's generally pretty good. So it's less of a risk to not get to his peer review before check-in. (* Not his real name)
- You might know that Joe* is new to the team and the code base, so you'll need to keep an eye on him. (* Not his real name)

You can't change people.
- Well, not much. Just accept that.
- But they can change themselves. If they're in the right state of mind, then you can help them. And if they want to change and understand why then it's easy. This is the best scenario you could want.

It's an unwritten requirement.
- This is a bit of a cop out, but, it's not a stated requirement. And yes, it should be. But if you can point me in the direction of something that I can use as a good metric, then let me know. (Don't suggest lines of code, or even cyclomatic complexity. These have their place, but not here.)

It'll probably come back from test.
- this is not a bad thing (unless it's too frequent), but you probably will have an opportunity to change it and test it again.

Peer Reviews don't have to be the only place where code is reviewed.
- There's no reason why you can't get a team mate to check over your work at any time.

It's not all about the code.
- This one might sound strange, but trust me, once you remove the blinkers that developers tend to wear, you will see that code quality isn't the most important thing. But it's damn close.
- Delivery of quality results for the business are paramount. If this means that the solution is acceptable, but the code should be refactored, but isn't, then so be it.

Oh, one more thing. I believe that I've found the right balance for the current team and requirements that I have to work within. If things don't work as well as I'd like, then they get changed. Continuous review and feedback.


Now, if you work in an environment where the quality of the code is paramount, like medical software, or missile systems, then these suggestions are not for you. Enforce your peer reviews relevant to your process.

Does anyone want to comment on this? C'mon, you know you want to...