November 4, 2016 | Written by: Colin Dean
Categorized: Community | Compute Services
Share this post:
This post is the third in a series on code review and architecture. Part one is What is code review and what problems does it solve? and part two is Quality attributes that code review ensures.
Tips for integrating code review
Here are some tips for making code review a part of your engineering process.
Integration in architecture planning
Conway’s Law states in paraphrase that the structure of an architecture eventually mirrors that of the developing organization, and vice versa, so it is vital that an architecture include code review as a not only a technical requirement at the same level as choice of language stack or inter-process communication methodology, but also as a project requirement that describes how the members of a project’s development team are expected to interact with each other and provide feedback in order to strengthen the project and the architecture behind it.
This is in line with a growing movement among software architects who believe that the architecture of a project must also consider human aspects and impacts, just as ethics, sustainability, and social contracts.
Execution of code review
According to “Best Kept Secrets of Code Review”, a case study at Cisco Systems found that there was negligible difference in the number of defects found when reviewing in a formal setting versus informally, for example via the individual review methods I described earlier, while informal reviews were faster and more cost-efficient. The ideal review size should not exceed 400 changed lines of code.
- Devote time: Code review should occupy units of work. That is, if your team is using story points or some other system to estimate effort necessary to complete a task, estimate the time necessary to conduct reviews. My team uses a Fibonacci story point system. Most of our reviews are one or two points. If it’s a big change or potentially controversial change with a lot of input to create and process, it might be a three or five.
- Accept debt: Not everything is perfect. A good code review system will enable reviewers or submitters to create new work items quickly and easily.
- Identify churn: Look for areas that see a lot of changes and understand why that is. There’s a whole field of study related to that, so don’t get too deep in it within the scope of reviews.
- Minimize pedantry: Discourage petty annotations: keep them professional and learn when to speak. Continuing to bicker electronically is waste of time most of the time.
- Make progress: Don’t let reviews slow down development unnecessarily. Dogpile reviews if they get backed up.
Here is a list of things that my team acutely watches for when conducting reviews.
- Algorithmic complexity
- Exception and error handling
- Exception, class, and variable naming
- Logging sufficiency and level
- Style conformation
- Long lines and methods
- Single purpose per commit
I must stress the value and importance of automation here. In early 2016, we automated our style checking on a new project and many of our most frequent corrections in code reviews went away: whitespace and formatting.
Sarcasm and humor often does not translate well through text!
Of course, those, the two main questions that must be answered are these: (1) Does it look like it will work? and (2) Does the submission include tests?
The former is obvious: if it doesn’t look like it will work, then it’s not ready or the submission’s description isn’t clear. The latter is less obvious, but drives test-driven development. In that philosophy, the tests serve as living documentation of how to use the code. Without tests, the code is less trustworthy and accepting that submission without tests increases technical debt.
Limitations of Code Review
Code review cannot analyze dynamic structures. It cannot really expose how code changes affect the runtime environment. A reviewer is certainly encouraged to download and execute the code as a simple verification if they feel that the provided tests are insufficient, but to do this constantly can greatly extend the review period and detract from that reviewer’s development time. Maybe one day there will be a neat tool that enables developers to interact with a change or a whole submission directly in the code review tool.
Code review cannot go on endlessly. Reviewers must learn when to communicate person-to-person instead of continuing a typewritten conversation. Drawn out electronic conversation is time consuming, and should only occur when code examples or hyperlinks are necessary to understand comments.
Most notably, code review cannot solve political problems. Code review will not magically get your boss off of you, or stop QA from knocking at your door. It won’t stop layoffs.
In fact, code review can be a barrier to one’s ownership of their contributions. A reviewer with an agenda could effectively block a submitter through combative or unnecessary annotations, or steal credit for their submission entirely through re-submission. One woman developer cited this as a part of her reason for leaving her company, later filing lawsuits alleging sexism. She held that a coworker she snubbed romantically blocked her work and would waste time altering it needlessly.
A counter to this specific problem? Ensure that no one reviewer is a bottleneck, or single point of failure, and that multiple reviewers can and do review submitters’ code.
You don’t have to take my word for it!
Are you not sold yet on code review, and only research can sate your thirst for knowledge?
Microsoft Research published a paper in 2013, “Expectations, Outcomes, and Challenges Of Modern Code Review”, a part of which asked developers for their motivations for performing code review, the definition of which matches very closely with what I established at the beginning of this talk.
From this, we can observe more empirically some of the outcomes from code review. All of these are desirable, thus code review should be desirable and a part of your process.
Go forth and review code
- Is systematic examination of proposed changes to a codebase.
- Solves mental model synchronization and tribal knowledge development.
- Ensures maintainability, compliance, & security.
- Must be short, thorough, and automated where possible.
- Will not solve all human problems, but some is better than none.
This series of blog posts is based on my presentation, Code Review is an Architectural Necessity. It was first shared at SATURN 2016 and subsequently at Github Universe 2016. This content is licensed CC BY-NC-SA 4.0.
Sign up for Bluemix. It’s free!