Style

If you’re talking purely about style, then it’s an excellent idea. Style rules are objective and so are the results: if style rule tells that the team uses four spaces to indent the code, a line of code which is indented with two spaces is a mistake. Clear enough.

Note that while style rules are objective (example: “The code is indented using four spaces”), choosing among contradictory rules is subjective and often hard (example: “Why four spaces is better than two? I disagree, …”)

• DON’T invent your own set of rules. Chances are you’ll spend weeks drafting them, and months discussing them with your team. Parkinson’s Law of Triviality works very well here: since most style decisions have no effect on the project and since there is no single right answer, the team can easily spend an enormous amount of time discussing style rules, or simply be blocked and stop the project.

• DO pick an existent set of rules among popular ones. For C#, it’s Microsoft’s style provided through the default rules of StyleCop. For JavaScript, it might be Google’s style guide. Other languages have other well or not well established standards which may or may not be commonly used.

  Picking an existent set of rules mitigates the risk of long, useless arguments. We use four spaces because Microsoft’s official style says it’s four. That’s all. End of discussion.

  It has an additional benefit: you can more easily find a person who either uses or at least knows the well-known style, compared to the chance to find a person who uses or knows the style you invented yourself (unless you work for Google or Microsoft).

  The last benefit: there are more chances to find a tool which already checks those popular rules. For example, StyleCop for C# or Google Closure Linter for Google JavaScript Style Guide.

The fact that style rules are objective also means that computers are good at making the checking, while humans, not so.

• DON’T expect people to check style themselves. They won’t, because it’s boring. Some may be inclined to match a given style and encourage others to do it, while others would try to behave as cowboy coders. In both cases, they will let a few errors into the repository, and, often under time pressure, would be inclined to stop checking.

• DO use a style checker through a pre-commit hook: no non-compliant code gets in, so it’s easy to keep the code base clean.

Note that setting style rules for an existent project which had no rules until now is a specific case which should be treated specifically.

• DON’T set a pre-commit hook for an existent project: setting such pre-commit hook on a code base which already contains a few thousands of errors would be a disaster both for the schedule and the morale of the team.

• DO fix style errors progressively in an existent project, and only then set the pre-commit hooks.

Code quality

If you’re talking not about style, but about code quality, any automatic rejection of code based on scores is out of question.

Code quality is a subjective measure and it involves a huge amount of factors. Measuring one aspect of it and enforcing the metrics through Continuous integration is a recipe for a disaster.

Example: a pre-commit hook makes it mandatory to limit the depth of the methods to two. A member of a team have written a method which is clear, readable code:

/**
 * Computes the actual price, based on the options which were selected during the check-in.
 */
public PriceInfo compute()
{
    Price amount = Price.createZero(Currency.Dollars);
    foreach (CheckInOption option in this.Options) {
        foreach (OptionVariant variant in option.Variants) {
            if (variant.Enlisted) {
                amount.add(variant.Price);
            }
        }
    }

    return amount.toImmutable();
}

Pair programming being used, the other programmer agrees the code is good enough. Developers commit the code and prepare to leave for a weekend, but a pre-commit hook rejects it, because it has a depth of three. What would be the reaction?

Chances are, the angry developer would split the method in two, creating a useless private method with not-so-clear name and the XMLDoc which paraphrases the method name.

/**
 * Computes the actual price, based on the options which were selected during the check-in.
 */
public PriceInfo compute()
{
    Price amount = Price.createZero(Currency.Dollars);
    foreach (CheckInOption option in this.Options) {
        this.addOption(option, amount);
    }

    return amount.toImmutable();
}

/**
 * Adds options to price.
 */
private void addOption(CheckInOption option, Price price)
{
    foreach (OptionVariant variant in option.Variants) {
        if (variant.Enlisted) {
            amount.add(variant.Price);
        }
    }
}

Now, the code is ugly as hell, and one can spend half an hour explaining how wrong it is. But the static checker doesn’t complain any longer, the commit can be done, and developers can leave for a weekend; wasn’t that the goal?

Same rule, another example:

for (var x = 0; x < matrixSize.x; x++) {
    for (var y = 0; y < matrixSize.y; y++) {
        for (var z = 0; z < matrixSize.z; z++) {
            // Do cool 3D stuff here.
        }
    }
}

Now imagine the exact terms the developer would pick when forced to modify this code to match the depth-two rule?

Automated enforcement of code quality has three issues:

• Chose one metric, and people will concentrate themselves on this metric, instead of the code quality itself. In the first example, the fact that I needed to concentrate on depth level caused me to omit readability, proper naming, proper commenting, proper use of object oriented programming, etc.

• Many things cannot be checked automatically with today’s technology. In the first example, no today’s checker would be able to tell that the newly created comment is useless (and even if it could, it would be easy to game).

• Hard limits are too rigid. In the second example, any code reviewer would accept a depth of three, because it simply doesn’t matter here. Most code reviewers would also accept three depths in the first example, while suggesting to refactor the code later if needed to get cleaner and more object oriented design.

Code quality is too complex and subjective to be done automatically; keep this task for people, though code reviews. This doesn’t forbid you from gathering metrics for statistical purposes during nightly builds.

I myself use some code quality tools, including Visual Studio’s Code metrics. I often noticed that my refactoring lead to drops of one or several criteria used by Code metrics, while the code itself was becoming more readable and maintainable. Those are only metrics: objective and reliable, but also blind and rigid. Use them as hints, not as something which tells you what to do.

You can set long-term goals, and use different techniques to encourage people to attain them. Fixing a goal of 90% code coverage for January 2015 and displaying in the hall of your company the photos of the developers who achieved the best code coverage according to the last commit stats is one thing. But forcing everyone to have 80% code coverage right now would only push developers to add useless tests of boilerplate code just to be able to get back to work, especially if there are good reasons for some teams to have less than 80% code coverage.

Personal considerations

Enforcing anything for the sake of doing it is usually a bad idea. Ask yourself:

• What is the primary issue?

• Is enforcing something the best solution?

Examples:

• You want to force developers to increase the code coverage. What is the primary issue? The fact that the number of bugs is too big compared to your expectations? Was your expectations realistic? While the number of bugs correlates well with testing, are there bugs essentially because of the lack of testing? Or because there are no written requirements, and developers don’t understand the needs of end users? Or because you hired programmers which are not skilled enough? Or maybe because there is too much noise at the workplace?

  Let’s assume that the only thing you can do to decrease the number of bugs is to increase the number of tests. Ask yourself, why aren’t programmers writing enough unit tests in the first place? Maybe it’s because management constantly pushes them to release faster? Maybe because they don’t understand the pleasure of TDD? Maybe because they believe that it’s up to Quality Assurance folks to write those tests?

• You want to set up some code quality checks. What’s the primary issue? The gap between the code written by best and worst programmers in the team? Wouldn’t pair programming help? Do you do code reviews? Can’t you simply fire programmers who are not skilled enough, and hire better ones?

  Let’s assume you can’t fire anyone and you can’t do code reviews or pair programming. But can’t you simply talk with the problematic programmers, explain them the issues, listen to their point of view? What if they are talented guys, but just hate to write PHP code? What if they simply don’t want to write high quality code, because management already cancelled two previous projects, and they don’t believe this one would survive?

You should probably mention the language. For Java there is Checkstyle (http://checkstyle.sourceforge.net/).

I have not used it personally, but colleagues said that it can be used in a continuous integration setup.

Maybe you can find something for your language in this article by searching for “style”:
http://en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis

I think automatically rejecting the code is not a good idea, though.
You need the programmers to commit to / agree on the coding style and make sure people react to a low score by fixing the style. I would still call the build successful (just with warnings). You probably do not want to work with programmers who refuse to go for conventions and a coherent style.
But there will be tense moments when you do not want the strict style checking to get in the way.

You should also look at the IDE integration for the tools. That might be a better place to be strict (better than the continuous integration server).