A former colleague of mine, Deon Thomas, posed a question in his blog post: “Can ‘agile’ software environments, truly accommodate decent code reviews?”. Deon asked me for my views, and I thought I would air them here, and give a full answer, instead of just leaving a simple comment :) Hope its not too long and isn’t too off track :D I have some time off until I start at my next employer, Entelect Software, so I have lots of time to write :D And I tend to babble :B
First, to answer his question, I believe that every team that develops software can gain tremendous value from regular, detailed, code reviews. I also believe it can be successful in an “agile” environment.
I’ll explain the very first project I was involved with as a professional developer and how the team ran, in my opinion, successfully in an “agile” environment.
My first project as a junior software developer was as part of a team of around 6-10 software developers (there was also a Business Analyst, Tester and Project Manager, as well as a DBA and other production support staff). The product was quite a mature online trading system, with a big and complex enough source base to keep the team busy (probably could have been a lot simpler though; there was quite a bit of spaghetti code, and a few little intricacies in the long ~15 minute build process, not to mention it was still stuck on Java 1.4 when Java 1.6 had already been out for a while).
Anyways, as a developer, you would have a few different responsibilities: development of new features, bug fixing and maintenance, production support, code review, and a kind of regression testing - you would test the software manually whenever you added a feature or made a change, to make sure you didn’t break anything (there weren’t many unit/integration tests, and those that were there hadn’t been run in years; the system wasn’t very amenable to automated testing). There were multiple branches in development at any point in time - since there was only one tester, development speed tended to be much faster than testing speed, and so only so much could fit into a release.
The flow of development was pretty typical (and I think I can remember them all :D). You would be assigned a task on the ticket/bug/issue-tracking system, you would investigate it and assign estimates of how long you expected it to take. After it was discussed with the project manager and/or team lead, you would then know when it was destined for release, and which branch to work on.
You would perform your work on a certain branch, and once you were happy it was working correctly (testing it manually, and capturing screen shots of your testing…), you would commit your code (as often as possible, preferably small changes as they occurred, or daily, but IIRC the body of work would be quite large and so larger commits every few days or after a week or so could/did happen too).
You’d then attach to the ticket a document detailing what your change involved, your screenshots for the tester to peruse at a later point in time, and I think also the branch and revision number. The ticket would be assigned to someone by a senior member to another developer for code review, who would add their comments on the ticket (either in a separate document, or as comments, or in the aforementioned document… can’t remember). Once you’d discussed what looks good or bad with the reviewer, and if you had to make any changes, you’d have a second round of code review to make sure the changes were applied correctly.
Since there were multiple concurrent branches, you’d also have to merge upstream (either before or after code review, can’t remember), and sort out any merge conflicts (which were very common, because of the spaghetti code and shotgun surgery). The ticket state would be changed to ready for testing.
When the branch was deployed for testing, the tester would perform regression/system testing, and when successful it would go through a User Acceptance Testing phase with the business users and the Business Analyst giving their stamps of approval before it would be readied for release.
A developer would have had to compile a deployment procedure document, and prepare the release artifacts. This would be reviewed again by another team member (usually a senior member), and then it would be handed over to the production support team. If the deployment was a tricky one, the developer that compiled the deployment procedure notes would also sit in on the deployment, or just be on stand by from home (the senior member would also be a backup stand by support person).
So, that’s quite a detailed explanation of the project’s development process, at least from my view. There would probably also be various planning and prioritising meetings that I wasn’t party to. But you get the idea.
I think one of the best parts of the project was that a developer would experience various roles in the development process: development of new features, maintenance involving bug fixing and refactoring, code review, merges, compilation of deployment documentation, deployments, test environment and production support (two people would be in the support role per week or every two weeks, using a round-robin type of assignment, to help production support with any issues as top priority, and to also help the tester with her environment).
Alright, to get back onto Deon’s question, I think that this project worked in an “agile” way. New features and bug fixes were developed in an iterative fashion. Tasks were prioritized and assigned to be performed for a certain branch/release (or, iteration), and only important pieces of work were performed first; if it wasn’t important to the business it would be left until future (or, put in the backlog).
The development pipeline was controlled using the issue tracking system, and each person in the team knew the correct workflow of the tasks: open > investigation > development > review > testing > UAT > closed. If any of those phases in the workflow had problems, another iteration of work for the previous phases would be performed, e.g. if the code review found problems, more development would occur before another review was done; only once the code review passed would it be merged and eventually released into regression/systems testing when necessary, and if a problem occurred during testing, the piece of work would probably require more development, review and then re-testing before it was sent to UAT.
This spiral model of development worked very well, and produced high quality software in my opinion. It worked also because each person on the team was highly capable of performing each of the tasks they were responsible for at the various stages in the project. If they didn’t have the know how, they were taught how to (and it was documented/updated on the project’s wiki). Proper prioritization was performed; estimation of tasks allowed for realistic goals for a release, and a manageable set of work.
Now, I think another reason that it worked was because there were quite a few developers. The work could be spread out so that one person wasn’t bogged down all the time - although, the more senior staff would be assigned quite a lot of work, requiring delegation if they did get bogged down.
I think the problem comes in with smaller teams, where you tend to always have a big list of tasks to do. Instead of focusing on improving quality, quality starts slipping as you try to deliver more and more features being asked from the business, with either unrealistic or highly demanding deadlines. This is also made worse when some of the resources on the team have to multi-task between different projects, or are stuck in meetings constantly: this impacts the project even more when those bogged down resources are required by other resources on the project, and then there’s so much contention that people aren’t as productive as they could be.
Trying to be more productive, the team will start to forego unit/integration tests, or do some testing but only look at one case (the success case, ignoring the multiple failure cases). Code reviews will be done less often, or in less detail, or not at all. And so on: less quality assessment procedures or measures will be performed.
One way of mitigating this is to enforce the workflow, and make it a team wide rule that everyone knows and follows, e.g. “No code fixing a bug will be committed unless a unit/integration test is also committed reproducing the bug. No code will be merged until all tests pass. No merge will be done unless all commits have been code reviewed.” If the transitions between the various states of a task are done by the issue tracking system, and it can verify that a transition is allowable, then even better.
Another way of avoiding this situation, is to improve planning by planning a truly manageable amount of work for the iteration. Ensuring that there is time allocated to unit testing and code review (whether as separate items with their own estimates and measures of time spent, or included in an aggregate estimate and progress measure), and that this is understood by the project manager, and handled by the PM if the client is asking why they aren’t getting more and more features - because the time is being spent on bringing less features with more quality. In the end, if you’re constantly bringing out quality code, you’re spending time on enforcing quality measures instead of wasting time fixing bugs that shouldn’t exist in the first place.
Finally, another great way of learning how a system works, and to learn its conventions, is to review the code. Sometimes even reviewing small code changes in commits can be much more enlightening than trawling through reams of code. It could also be used to keep track of the team’s progress, and enhance a daily/weekly stand-up/progress-meeting.