In my enterprise we recently introduced Test-Driven Development (TDD) as a programming technique for delivering robust and reliable software.
The lifecycle of tasks is the following:
The “To Do” and “In Progress” phases are self-explanatory. When a task goes in “For Review” the developer choose, with a round-robin approach, one of the team mates, and together they review the code in order to ensure quality (eg. check the code is clear, correct design and design patterns, high code coverage). Last but not the least it comes the “Verified” phase, during which the developer ensure that new features behave as expected in System Test.
There have been serveral attempts to make the “For Review” phase appealing, interesting and efficient. After some meeting and discussions we came up with the following check-list.
- Overall Design
- Development Prctices
- Error Handling
Even if we, in my humble opinion, perfectly described what should happen in a review phase, when we put it in practice, results were not so exciting as we were expecting.
What you really want to achieve in a code(peer)-review, is to ensure that the code is clear, well designed and correctly tested (in a TDD fashion!). What we were doing was to make the developer lead the review process and tell what the code was doing. Under these circumstances a reviewer would make zero or nearly no effort in understanding what the code was actually doing. Finally the “code coverage check” simply turned into a “check sonar code coverage %”. This could guarantee in really few circumstances that the quality and clarity criteria were met.
Since the TDD approach expects tests to be written before the actual code, why reviews should not be structured in the same way?
Few days after I finished to read the “Clean Code” book, I decided to play a game with a colleague for testing how well I learnt principles uncle bob explained in his book.
I asked him to guess, just reading from a bunch of test classes, what my code would have done. The result was surprising. With only a few variables and methods renaming, my colleague was able to correctly guess what my code was trying to do.
The review process honestly started very slowly because I, the developer, was giving as less hints as possible and was letting my colleague to progress alone as much as he could. Once sorted the initial naming incongruences, the review itself turned into a vivid and exciting conversation.
We agreed that
- the code was clear because he could guess with a little effort and nearly no suggestions what the code was trying to do.
- the code was well designed because class hierachies and patterns I used were thoughtful
- I correctly followed the TDD approach because I guaranteed a very high code coverage
What I learnt that day is that in order to understand whether or not you wrote clean and well tested code is to simulate somebody to work on your code in a day you’re off. If he/she’s able to do that without ringing you.. well.. you did a good job!
Thoughts on the review process
- Reviews have to be interesting. If the review process turns into something boring, it will not give results you expect. This can be achieved challenging your reviewer!
- In a review process the reviewer is the one who has to talk the most, if it is the developer, well, you have a smell there!