Click here to Skip to main content
Click here to Skip to main content

Everyday Challenges to Improving Brownfield Code Quality

, 8 Jun 2009 CPOL
Rate this:
Please Sign up or sign in to vote.
How best does one integrate code quality tools into an active project?

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

CQT Type

Visual Studio / C#

Eclipse / Java

See http://www.ibm.com/developerworks/java/library/j-ap01117/

Style Enforcement

StyleCop

http://blogs.msdn.com/sourceanalysis/

CheckStyle

http://eclipse-cs.sourceforge.net/

or Eclipse built-in Java Code Style options.

Static Analysis

FxCop

http://blogs.msdn.com/fxcop/

PMD

http://pmd.sourceforge.net/

FindBugs

http://findbugs.sourceforge.net/

Duplicate Code Finders

DuplicateFinder

http://www.codeplex.com/DuplicateFinder

CPD (part of PMD)

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/

License

This article, along with any associated source code and files, is licensed under The Code Project Open License (CPOL)

Share

About the Author

sdesciencelover
Software Developer
United States United States
No Biography provided

Comments and Discussions

 
Generalrestyle then fix PinmemberM Towler18-Jun-09 6:00 
GeneralRe: restyle then fix PinmemberDale Visser18-Jun-09 9:16 
GeneralAn alternative approach PinmemberStephen F. Heffner8-Jun-09 15:52 
GeneralRe: An alternative approach PinmemberDale Visser16-Jun-09 6:36 

General General    News News    Suggestion Suggestion    Question Question    Bug Bug    Answer Answer    Joke Joke    Rant Rant    Admin Admin   

Use Ctrl+Left/Right to switch messages, Ctrl+Up/Down to switch threads, Ctrl+Shift+Left/Right to switch pages.

| Advertise | Privacy | Terms of Use | Mobile
Web01 | 2.8.1411022.1 | Last Updated 8 Jun 2009
Article Copyright 2009 by sdesciencelover
Everything else Copyright © CodeProject, 1999-2014
Layout: fixed | fluid