NDepend on a Real Project (Again): My Critical Errors

Following on from my look at the non-critical errors NDepend 6 found on my project, let's look at the more serious stuff - the critical errors. Our old friend the Queries and Rules explorer shows me this:

CriticalErrors

So - mutually dependent namespaces, uncalled methods, and types with the same name as other types. Taking these in reverse order:

SameNamedTypes

…the third rule has matched ExpressionExtensions and TypeExtensions. As an aside, it's worth mentioning how elegant the rule definition language is and how easily you can express complex rules. Look at that! It's great!

As you might expect, ExpressionExtensions and TypeExtensions contain extension methods for Types and Expressions respectively. I only have one class of each name, so where's the conflict? Clicking '2 Types' tells me:

TypeExtensions

It's conflicted with another project of mine on which the mapper depends - ReadableExpressions - which creates a friendly view of an expression tree. So how big a problem is that?

Naming things is hard, and following naming conventions is almost always a good idea. If I'd made a class named StringBuilder this rule would apply, but I'd say these are a special case. You don't even use extension method classes directly - you import the namespace and use the methods - so it's not going to conflict with anything. With that in mind, let's update the rule:

SameNameTypesUpdate

Look how easy that was!

Moving on to the dead methods:

DeadMethods

I've got the rule description view selected this time instead of the query - this is a new version 6 feature and is again typical of how helpful the tool tries to be Smile 

The first 4 of these methods are called, via reflection - the fifth one I copied from an earlier incarnation of the project and will be called in the future. As I mentioned before, finding methods which are called via reflection is going to be a very awkward task, but… hang on, we've got test coverage data! So if we update the rule:

CoveredMethods

We can easily exclude anything which has coverage. Using GetValueOrDefault() in the query also keeps it relevant if there's no coverage data available.

So finally - those mutually-dependent namespaces again. Here's the details:

MutuallyDependent

You can see that all but one of the violations is for the AgileMapper or AgileMapper.Configuration namespaces. Most of the links to the former are to a Constants class and to the latter a class exposing user-configured settings. This type of coupling can be solved by inversion of control (as detailed in the rule description in the image above) - should I do that here?

Constants is a static class with all constant or static readonly members. It's a pure convenience class enabling (for example) Constants.PublicStatic to be used in place of BindingFlags.Public | BindingFlags.Static all over the place. In theory, I could create an interface in (for example) AgileMapper.Extensions, and implement it in a class in AgileMapper, passing an instance to the AgileMapper.Extensions namespace to break the coupling. The configuration class is used everywhere as user-configured settings apply to lots of scenarios. In theory, I could create an interface in (for example) AgileMapper.TypeConversion, and have MappingConfiguration implement it, breaking the coupling. At this point I want to revisit my two questions regarding rule violations:

  1. Do I agree with the rule?
  2. Is the cost of fixing it worth the improvement?

I certainly agree with the rule in many scenarios - abstracting some types of components is vital to testable code. Are these examples worth fixing? I don't think so. In order to abstract the configuration class, each currently-coupled namespace will need its own interface - this would leave MappingConfiguration implementing 5 interfaces just to break the coupling. Am I ever likely to want to move MappingConfiguration into a different assembly or replace it with an entirely new implementation? No, and the same is true of Constants. I remember seeing a long time ago discussion about how far to go with abstraction and decoupling, and I think these would be steps too far. So do I want to switch the rule off? No. I think it's an important one to keep an eye on, but like so many of these rules, you have to apply common sense.

Which brings us to the end of my look at NDepend. Ultimately it's a fantastically useful tool delivering insight and information, which can't be a bad thing. Run it on your own project(s) and see what you find!

Print | posted @ Thursday, February 11, 2016 10:23 PM

Comments on this entry:

Gravatar # re: NDepend on a Real Project (Again): My Critical Errors
by Patrick Smacchia at 2/16/2016 1:28 PM

>I certainly agree with the rule in many scenarios - abstracting some types of components is vital to testable code. Are these examples worth fixing? I don't think so.

It makes perfectly sense to not 'fix/change' this particular.

On the other hand having a property on your code structure as strong as: 'there is no dependency cycle between namespaces in my code base, no exception to that' is super useful to keep the whole structure sane and maintainable in the long term.

So by fixing all reported cases, even edge cases like this one, you'll get the benefit of living with this strong property + the additional benefit of making this property verifiable each time NDepend rules are passed.
Post A Comment
Title:
Name:
Email:
Comment:
Verification: