One of the practices that’s become semi-common within development teams over the past ten years or so has been the observance of formalised management/peer reviews of team members’ work, known as Code Reviews. The specific format these reviews take, and who carries them out, varies quite a bit from workplace to workplace, and project to project. This article covers some of the approaches I’ve encountered within various working environments, and provides some personal viewpoints on what works and what doesn’t when carrying out reviews.
When I first started writing software professionally, in the late 1980’s, it was a fairly solitary activity. Development ‘Teams’ tended to be comprised of at most two or three individuals. Often, only one of those people would be writing the actual code, and it was rarely the case that anyone other than that single coder would take an interest in the actual implementation of the solution (as distinct from a user-level understanding of the functionality being developed). Code reviews were therefore fairly superfluous to the development process – solutions either worked, or they didn’t, and it was usually fairly easy to see what was or wasn’t working without having to enter into too much analysis of the subject.
In recent years, since around about the turn of the millennium, it’s become more popular to have larger development teams, comprised of more specialised (and less broadly-skilled) individuals. The reasons for this change in approach are many and varied, but perhaps one of the main drivers is that there are so many more development technologies available now in comparison to those early days, and having larger teams allows different parts of those larger and more diverse teams to better focus on more specific parts of the solution. It’s common nowadays to find 3-4 developers, a DBA (sometimes more than one, especially in the Public Sector), Business Analysts (including some that don’t have a coding background), Project Managers, and specialist users with expert knowledge of the problem being solved, all working together within dedicated teams aimed at producing a single software product. There are some advantages to this more modern approach, including greater support for individuals within the team and an increased ability to fill gaps left by any departing members, though there are some disadvantages too. Specifically, a corollary of working within those larger and more diverse teams is that, when working on multiple fronts consecutively, there is more chance of any one particular area of work getting out of sync with the rest of the team’s activity. By increasing the separation of concerns, development teams also multiply their potential points of failure. This is where Code Reviews come in.
Code Reviews, when done correctly, can have multiple benefits to a properly-functioning and well-motivated team. They can help individuals to communicate their ideas with, and engage in mutual learning from the experience of, their peers and technical management, thereby facilitating a two-way process of learning and mutual understanding. They can make the product being developed more robust and maintainable, since it stands to reason that the more members of the team that know about the logic behind the wider implementation (including parts of that implementation they may not have worked on directly themselves), the easier it’ll be for any one developer to take over another’s work if the demands of the team (sickness, holidays, re-prioritisation, etc) dictate. Perhaps most importantly, though, Code Reviews enable individual developers to understand the impact that their own work has within the context of the wider team’s direction. They enable individual members of the team to contribute their own part of the wider effort in conjunction with rather than in isolation from the concurrent activity going on around about them. In this way, they encourage each member of the team’s work to be part of a widely-understood set of greater goals, rather than just leaving each individual to contribute their part of what eventually becomes a patchwork quilt of loosely-aligned ideas.
If carried out incorrectly, on the other hand, Code Reviews can be extremely damaging to a team’s morale. If conducted in the wrong spirit, advice passed on during code reviews can seem, from the perspective of the recipient, to merely represent unnecessary interference in, unwelcome distraction from, and unjustified criticism of, their work. They can create a climate of fear that stifles individual initiative. If done badly enough, they can breed resentment that ultimately leads otherwise skilled people to (correctly or not) conclude that they can’t do right for doing wrong, sapping their confidence and motivation, and diminishing the team’s productivity.
So, how best do them properly? Well, in my humble opinion from two decades of being on the receiving and delivering end of same, here are some of the things that I’ve seen work worst and best during my career:
1) Keep It Private
I’ve found it best to always give reviews about individual developers’ work directly to the person concerned, face-to-face and in private. If more than one individual is involved in a particular piece of discrete work, it’s of course fine to have a conversation together with all of the parties concerned aside from the wider team, but any discussion of individuals’ work should never be done in front of uninvolved members of the team. It’s human nature to perceive even constructive criticism as a negative experience, where that constructive criticism is delivered before an audience of the recipient(s)’ peers. The self-same feedback that would be perceived as well-meaning advice if delivered in private, can sound like the harshest criticism if given in public; context is everything. I once knew a lead developer that not only gave code reviews in public at individual developers’ desks in front of the whole development office, but who also later wrote about those individuals’ work in clearly-identifying terms on his internet-facing public blog. I guess he must have mistakenly thought of the internet as an anonymous medium and considered his blog in much the same way that other people might think of their private diaries. Ouch. My advice is to never behave in this way, however tempted you may be to vent frustration and however much you may feel that an individual deserves it. Criticising your peers (or, even worse, your direct reports, who look to you for even-handed guidance and leadership) in public is never a classy way to behave. Doing so is guaranteed to immediately lose you credibility with the very people whose work you should be trying to positively influence – who wants to work for someone that appears only to be interested in embarrassing and deriding them in public? What you should aim to achieve out of each Code Review is a more motivated and better-informed team member, and an insight into that team member’s thought processes for yourself; it’s plain to see that you’re unlikely to achieve either of those outcomes by merely seeking to criticise or denigrate people in public.
Code Reviews are either Win-Win or they are Lose-Lose for the parties involved; by providing any constructive criticism you have to make in private, you’ll help ensure the former rather than the latter outcome.
2) It’s Not All About The Negatives
One of the things that can make Code Reviews become a morale-sapping chore rather than the productive exercise they should be is if a perception is allowed to develop amongst the team that they’re merely used to provide oversight of those being reviewed, to pick up on any areas where there’s room for improvement. For experienced technical professionals (who are most often experts in their particular field, and are regularly the most experienced member of the team in whatever it is that they individually specialise in) this can sometimes feel like a wholly-negative experience. Even where the technical reviewer is of equal experience in a given technology, it’ll certainly be the case that the person being reviewed has a greater knowledge of the solution as it was developed, since they will be the very person that built the thing. They’ll know precisely why they chose implementation ‘X’ over possible alternative ‘Y’, and can resent it if someone with ten minutes’ understanding of their work chooses to focus exclusively upon perceived negatives in preference to recognising the bigger picture. It’s not that reviewers should be afraid of giving relevant feedback where necessary – they should by all means do so, that’s one of the things they’re there to do – but it is highly beneficial to the process if that feedback can be delivered in a way that avoids alienating the skilled people whose initiative and expertise the reviewer should be seeking to encourage and nurture. One of the key things that makes for such an effective review is that the reviewer should make clear that any constructive criticism being passed on is being given within the context of a generally acceptable performance by the person being reviewed (if indeed that is the case, which it should be in all bar exceptional circumstances: if the reviewer was responsible for mentoring and briefing the person whose work they are now reviewing, and the reviewer has been keeping an eye on source control during the development process, any major problems discussed during a code review shouldn’t come as a total surprise to either the reviewer or the developer – unaddressed major problems should have been spotted and raised well in advance of a retrospective code review). Showing appreciation for the many positives of your direct reports’ efforts within the wider context of a rounded and balanced review of what they have produced goes a long way to lending credibility to any suggestions for improvement you may have to make.
3) Don’t Nit Pick
4) Use The Review To Communicate Across The Team
I mentioned above that one of the most beneficial aspects of Code Reviews are that they allow individuals within the team to gain a broader perspective on where their work falls into the bigger picture of concurrent activity going on within the wider team. Code Reviews can be a good time to discuss other areas of the wider solution with developers that are uninvolved with those areas, and to explain where the work being reviewed is intended to fit in with those separate parts of the wider team’s activity. This helps provide an element of useful redundancy within the team (in case developers need to cover for one another or take over one another’s responsibilities for a time), and helps communicate a greater understanding of why certain strategic decisions have been made. From the perspective of the reviewer (who will generally be the lead developer or development manager), Code Reviews also help communicate intent from individual developers back to those guiding the overall solution; as touched upon earlier, good reviews should involve a two-way flow of useful information and feedback.
5) Write It Down
Code Reviews should be formally documented, and held confidentially for later reference only by the reviewer and the person whose work was the subject of the review. This formal record of the review should ideally only be 1-2 pages long, will contain less than 500 words in most cases, and no more than 1000 words in any event. The written outcome should be a list of bullet points of any positive and negative observations to be raised, that the reviewer brings two copies of to the meeting and uses to guide the discussion, then e-mails to the person being reviewed after the meeting. It’s advisable not to send this document in advance, since the brief nature of a bulleted list can be taken out of context if the issues it pertains to aren’t given context by being elaborated upon in person. Another reason for waiting until after the meeting to mail the document to the recipient of the review is that it allows you to incorporate any feedback from the meeting itself into the formal record, and to note any specific outcomes that are expected (such as any remedial work that may have been requested). If you’re feeling brave and you’d like to encourage openness and communication, it’s also an idea to include a section at the bottom of the document headed : “And how did I do?”. This encourages the recipient of the review to give you any feedback they wish on any aspect of the process. Any given project needs to have one distinct individual that is entrusted with responsibility for the overall direction of the solution, and that individual will usually be the person conducting the code reviews (though I have seen working environments where developers have been asked to review one another’s code on a regular basis, which I don’t think worked particularly well - developers like to be able to focus on their own work in the main, and they often perceive it as a distracting and unnecessary chore if they’re instead tasked with regularly commenting on their peers’ work; perhaps most importantly, though, developers’ time is too valuable to waste on administrative tasks that the technical leadership of the team should be shouldering for themselves). Meaningful communication between members of the team is the key to making sure that the main technical decision maker knows everything they need to know to do their job effectively. If communication is truly to be bi-directional, in a code review it’s advisable to document any feedback from the person being reviewed alongside any critique the reviewer may have made of their work.
So much for Code Reviews, and the best and worst of how I’ve seen them employed in various places. I of course welcome any reviews of the above that anyone out there may care to make. ;)
