Take the 2-minute tour ×
Programmers Stack Exchange is a question and answer site for professional programmers interested in conceptual questions about software development. It's 100% free, no registration required.

For several years now I am a big fan of using static code analysis tools for checking the source code quality. We are mostly doing C# development so NDepend was the best way to go for me. Most of the time I manually perform the analysis, for example once every two weeks.

Lately I have had a discussion with a college whether we should include NDepend in our continuous integration process so that the build automatically turns red if for example a method has a too high complexity.

My opinion hereby is that it's not possible to define metrics that are valid for the whole code. I think that you would either get way to many "false complaints" - code that generally is ok but does not match the metrics. The following code for example has a very high complexity, even if I think that sometimes you can't prevent it.

if (remoteResult == RemoteResult.Dummy1 || remoteResult == RemoteResult.Dummy2 || 
    remoteResult == RemoteResult.Dummy3 || remoteResult == RemoteResult.Dummy4 || ...

Of course it would also work to increase the thresholds of the metrics, but in this case I would loose valuable information.

Another example for example is the maximum number of members for a class. I would like to keep this lower than 10 for classes with lots of logic. But on the other hand there are many "domain models" with more than 10 properties. I wouldn't like to extract another class just because of the metric - as this would lower the readability and does not improve it which should be the goal of code quality metrics.

Do you think automatically checking code quality via metrics is possible? If yes, for all or only for specific metrics? (A list of common NDepend metrics can be found at http://www.ndepend.com/Metrics.aspx )

share|improve this question
2  
Define "quality" first... Your criterias may not be the same as others. –  user1249 Jun 14 '12 at 19:38
    
That must have been quite a large conversation with an entire college... –  Reid Mac Jun 14 '12 at 20:29
    
Yep, I'd say that's higher complexity than necessary. I like this more: if (remoteResult in [RemoteResult.Dummy1, RemoteResult.Dummy2....])... –  Izkata Jun 14 '12 at 21:04
add comment

5 Answers

up vote 8 down vote accepted

It is definitely impossible to automatically check code quality, you can only check for some minimum code quality.

Even "experienced" programmers are having trouble to formalize what actually constitutes code quality. And even among the "most skilled" ones, there's a high divergence in opinion on what is good or bad.

There are some obvious rules that shouldn't be broken. A thousand lines in a method is definitely too much. A thousand members in a class is also. Nobody would argue on that. Most people would choose for smaller numbers. I personally rarely have a file with more than 200 LOCS, but it largely depends on personal taste, habit and also the programming language used.

The thing is, automatic code analysis tools can only use rather simple criteria. Therefore, not unlike testing, they can never be used to ascertain code quality, only determine horrible lack of it. If there is an absolute way to define code quality, I would think it means a sensible ratio of complexities between a solution and the problem that it solves.

However you do not care for code quality as such and do not seek its absolute value - well, you personally may (I hope you do), but the people who pay you generally don't care. What you really care for is maintainability and ideally a low bus factor.
And this is something relatively subjective, depending a lot on the people in your team. They need to be able to look at a piece of code and understand what it is doing. And that can ultimately only be achieve through code reviews.

So while you can implement automated means to uncover obviously bad code, you cannot implement such means to really assure good code. Only code reviews can do that (or at least drastically help getting towards that goal) and they also definitely protect you from code that is obviously bad.
So it really depends on your team (size, continuity, etc.), whether or not such an automated check makes sense. It might as well save time to do more important things in reviews. Or it might just piss people off to no end. In the long term you should find the discipline to not commit super-obviously-bad code in the first place. It's really not that hard and it really pays off.

share|improve this answer
    
Woah, I posted my answer before I even read yours. You said it, word by word. Deleted mine, +1'd yours. –  Yam Marcovic Jun 14 '12 at 20:53
    
+1: There are a few cases where multi-thousand-line functions can be necessary (e.g., a bytecode engine core can get that large) but they're pretty rare. –  Donal Fellows Jun 14 '12 at 21:02
add comment

I think that it's certainly possible.

However: it should never break the build.

I don't know of software that integrates with CI out of the gate for this type of static analysis (the way that FxCop does), however, I think they can/should be raised as warnings that should flag those areas for code reviews. Upon review, those warnings can be suppressed if code review appoves the code. Similar to how FxCop and StyleCop have suppressions in the source/globalsuppressions.

In summary: these metrics are better as code smells, not code errors.

share|improve this answer
add comment

I use static analysis & linting tools as part of my CI and build process, and I find it both helpful and useful. There are drawbacks, but the added confidence that you get in the quality of the code makes it worthwhile, IMHO.

There have been quite a few occasions where the static analysis has rejected functions of mine that I would otherwise have considered quite reasonable; for example, too many variables, or too many input arguments, excessive McCabe complexity etc... However, I have always been able to take a philosophical view, and refactor the code into compliance.

As a result of this approach, I feel a lot more comfortable with my code-base, and can move a lot faster when I need to make changes.

The primary benefits are psychological, and they are significant and meaningful.

(For me, the key to getting it to work well is to use the static analysis tools right from the very beginning, as refactoring legacy code to meet code quality standards is a pain in the backside, whereas continuing development of already-compliant code is much easier).

share|improve this answer
add comment

No

In my opinion it's far more trouble than it is worth. If you have mediocre programmers, they will slavishly refactor their code to satisfy the style cop, making the code even worse. If you limit the number of class members, you will get something like this:

class BunchOfStuff {
  public class Bunch1 { // properties with names from A-M
     Integer a { get; set; }
     Double b { get; set; }
     ...
  }
  public class Bunch2 { // N-Z
     ... 
  }

  Bunch1 bunch1 { get; }
  Bunch2 bunch2 { get; }
  ...
}

I've seen it. In response to an automated check for 'magic numbers', at least one developer complied thusly:

private static final int SEVENTEEN = 17;
...
Something[] things = new Something[SEVENTEEN];

If you want maintainable code, there is no substitute for good judgment and a second pair of eyes.

share|improve this answer
add comment

Metric only aren't enough, but can provide great informations about the code. Just like every metrics, they have to be interpreted to have meaning.

Consider a table which is 2 meters long. Is is too much ? Not enough ? Actually, it depends on what you want to do with the table.

As well, you expect most of your code to have a low cyclomatic complexity, but it is expected that algorithms have a bigger one than regular code. So depending on the code, what is a good and a bad value can change.

As metric, you should look at :

  • NPATH (measure the logic complexity of a piece of code).
  • CRAP (a metric that combine cyclomatic complexity with code coverage to tell you how risky it is to change a given piece of code).
  • Data abstract coupling, fan in and out (to have an idea of the quality of the isolation within an application).

Some problem will not be revealed by any metrics I know of. For instance, temporal coupling. Metric cannot be a substitute for code review, or any quality process you have in the company. This is, however, a great addition and can help in the task.

By the way, Sonar provide what you need to automate code metrics measurement and make report. Check it out : http://www.sonarsource.org/

share|improve this answer
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.