Click here to Skip to main content
Click here to Skip to main content
Go to top

Everyday Challenges to Improving Brownfield Code Quality

, 8 Jun 2009
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 5:00 
GeneralRe: restyle then fix PinmemberDale Visser18-Jun-09 8:16 
GeneralAn alternative approach PinmemberStephen F. Heffner8-Jun-09 14:52 
I agree that style standards are important. As you mentioned, they eliminate minor squabbles, and enhance code's readability across the enterprise. My feeling is that those who object to an imposed style need to concentrate on exercising their creativity at a more meaningful level -- say, architecture, design, and quality implementation.
 
I am the author of a software engineering meta-tool that can automate many of the issues you raised -- code analysis for both style and quality, and the re-engineering needed to bring code into conformance (not to mention translation to other languages if needed). Using the same meta-tool, with the same rules language, for both analysis and re-engineering allows a consistency of approach and automation of the conformance effort, which eliminates the possible objection you raised about style changes obscuring content changes.
 
An example: We had a client with about 600K code lines of PL/I. We delivered to them a code quality and improvement package, based on our meta-tool, that replicated the functionality of an existing tool they liked that, unfortunately for them, could only handle COBOL. The rules included:
 
o Analysis rules to provide "demographic" measures such as statement counts
 
o Analysis rules to provide quality measures such as Cyclomatic Complexity, Halstead's Volume, GOTOs, exits, knots (GOTO crossings), and dead code
 
o Analysis rules to provide cloned code detection at the PROCEDURE level
 
o Re-engineering rules to eliminate "GOTO <label>"s, which confound code structuring
 
o Re-engineering rules to structure the code, imposing IF/ELSE and DO statements to eliminate GOTOs
 
o Re-engineering rules to eliminate remaining GOTOs using "unrolling", with creation of local PROCEDUREs to avoid code replication
 
o Re-engineering rules to change long IF / ELSE IF chains to SELECT statements
 
o Re-engineering rules to "flatten" deeply nested code by extracting nested code as local PROCEDUREs, enhancing readability
 
I created all of those rules in a matter of weeks.
 
When we ran the rules on the client's code, we discovered that potentially forty per cent of the code was cloned! The client was not surprised; they had an ingrained habit of copying and pasting code from many years ago.
 
They said they couldn't improve and declone their code all at once; it might not survive the effort, and they didn't have the budget or time for such a massive effort anyway. So they were contemplating the possibility of improving and decloning each module as it needed to be touched for any other reason, which would give them an incremental improvement process.


 
Stephen F. Heffner, President | Phone: +1(480)626-5503
------------ | Fax: +1(480)626-7618
Pennington Systems Incorporated | Email: Heffner@Pennington.com
8655 East Via de Ventura, Suite G200 | Web: http://WWW.Pennington.com
Scottsdale, Arizona 85258-3321 USA | XTRAN: A software engineering meta-tool

GeneralRe: An alternative approach PinmemberDale Visser16-Jun-09 5: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 | Mobile
Web03 | 2.8.140916.1 | Last Updated 8 Jun 2009
Article Copyright 2009 by sdesciencelover
Everything else Copyright © CodeProject, 1999-2014
Terms of Service
Layout: fixed | fluid