Everyday Challenges to Improving Brownfield Code Quality
It is my belief that Code Quality Tools should be integrated into the code review process and that all code checked into a project should pass bars set by such tools. This article will define what I mean by Code Quality Tools, and discuss the challenges teams face when using them as part of the code review process.
There are at least 3 classes of tools that I am designating here:
1. Style enforcement tools
2.Static analysis tools
3. Duplicate Code Finders
In the rest of this article, I will discuss in terms of style standards. However, the exact same arguments apply to when teams standardize on static analysis tools or tools that call out sections of duplicate code. (I would love to find a free duplicate code finder tool that integrates with Visual Studio, but am currently unaware of one. DuplicateFinder claims to integrate with MSBuild, though.)
Pros and Cons of Style Standards
When your team standardizes on a set of style rules for your code, it removes pointless debates about coding styles and conventions from the code review process. Code reviews can then focus more heavily on the higher level issues of correctness, maintainability, performance, design and so on. Additionally, having a predictable style that is the norm for the entire code base improves overall readability for anyone who must read the code.
There will always be differences of opinion on a team over the best style conventions to use, and very few will agree 100% with the standard. It is necessary, then, for conformance to be a “commandment from on high”. Arguments over whether this rule or that rule makes sense are time wasters when it comes to producing working code. That said, there will always be corner cases where following the tool creates more headaches than it’s worth. This is OK. File a bug with the authors of the standards tool and move on.
The Issue
Consider the usual case where there is an existing codebase, and a well-meaning developer has convinced his team leader that the code style should be standardized according to a tool we’ll name ClearStyle. ClearStyle has plug-ins that integrates it with all the various team members’ favorite IDE’s and text editors. In addition, it can integrate nicely with the team’s build system in order to give warnings of code written in a “deviant” style.
The amount of code is too large, and the pending feature work too important, to devote anybody to bringing the code all into ClearStyle compliance at once. An agreed-upon strategy has to found for gradually bringing into compliance those files where code churn is happening.
One of our team members, who we’ll name Katie, is working on a fix for Bug X, which was assigned to her. For the sake of argument, the fix is in an active branch of development and touches one code file, ComplicatedLogic.cs, which is badly out of ClearStyle compliance. She has a few options:
1. Bring ComplicatedLogic.cs into compliance along with your bug fix. She’ll have her reviewers crying foul that they can’t easily separate the bug fix from all the style-related changes, making the code review too difficult.
2. File a work item in the issue tracking system that there is a need to bring the specific file into compliance, and either
a. Do the style fixes, get them reviewed and checked in prior to changes for her bug fix, or
b. Prioritize her bug fix by working on getting it checked in without style changes. Hope that she or someone else will actually fix the “style bug” instead of resolving it Won’t Fix before very long.
Of course, for branches that are simply for releasing hot fixes and the like, I wouldn’t recommend worrying about style issues. There, the bug fix itself clearly takes highest priority.
I would love to hear comments about how this is tackled by teams elsewhere. Is there other options I haven’t thought of? I have heard that at Google, they take style standards seriously, but I haven’t heard what their process is in detail.
History
06/07/2009 Original Post, Cross-posted from http://ars-excellentia.vox.com/
