Along with unit testing, continuous integration, and code review, static code analysis is invaluable for maintaining healthy code base.

Ignite.NET uses FxCop and ReSharper code analysis from early days, and this process is included in continuous integration on Ignite TeamCity, so I was fairly confident that our code is (mostly) fine.

Not so long ago Patrick Smacchia reached out to me and suggested to try NDepend as well. Let’s see how it works out!

Getting Started with NDepend

We are going to work only with Apache.Ignite.Core project: it is the biggest, most important, and most complicated part of Ignite.NET.

NDepend, like FxCop, operates on a built assembly (dll file), so we have to build the project in Visual Studio, open up the assembly in VisualNDepend.exe, and hit F5 (Run Analysis). The process is surprisingly quick: 1 second on my machine, where ReSharper (command-line) and FxCop take several seconds.

Upon analysis completion we are presented with some statistics and a summary of issues:

2654 issues! Whoops! No blockers at least. But every static analyzer produces false positives and irrelevant issues, so it’s not time to worry, yet.

Exploring Code Issues

There are 4 critical issues and 7 critical rule violations, we are going to start with these. Click on red 4 number to reveal a list of issues:

Avoid types initialization cycles (3 issues)

Hovering over the “Avoid types initialization cycles” rule name shows a pop-up window with detailed rule description along with possible false positive scenarios.

Our case seems like a false positive, since BinaryObjectBuilder does not have a static constructor and does not use BinarySystemHandlers in static field initializers.

The only usage of BinarySystemHandlers is on Line 127, but this has nothing to do with type initialization.

However, looking closer at how these three classes (BinaryUtils, BinaryObjectBuilder, BinarySystemHandlers) relate, reveals a real code quality issue: method GetTypeId does not really belong in BinarySystemHandlers, it operates only on some constants from BinaryUtils. So we could move GetTypeId to BinaryUtils, but that class is already ugly (“utility class anti-pattern”). Proper solution is to extract type code handling logic to a separate class: IGNITE-6233. So even a false positive turned out to be quite useful.

Don’t create threads explicitly (1 issue)

This issue is simpler, new Thread(Run).Start() should be replaced with a ThreadPool or Task: IGNITE-6231. Exception handling is also missing in that thread, which can potentially bring down entire process.

Moving on to High issues.

Avoid namespaces mutually dependent (89 issues)

The most violated High rule. It tells us that low-level namespaces (such as Apache.Ignite.Core.Impl.Binary) should not use higher-level namespaces (Apache.Ignite.Core). Following this rule provides a proper layered architecture where each namespace is kinda “thing in itself”, which can be moved anywhere, extracted to external assembly, etc. This makes refactoring easier.

Such decoupling can be achieved by introducing additional interfaces. In real world, however, introducing an interface just for the sake of decoupling within single assembly is not feasible. Most of the core Ignite functionality is quite tightly coupled and is not expected to exist separately.

Good example of this is serialization engine: it is inseparable from Ignite, because it exchanges type metadata with other nodes separately.

This rule is still useful for baseline comparisons.

P/Invokes should be static and not be publicly visible (21 issues)

IgniteJniNativeMethods class is internal static, so this looks like a false positive to me.

A field must not be assigned from outside its parent hierarchy types (17 issues)

This is a very good rule, and Ignite.NET coding guidelines go even further by disallowing non-public fields entirely.

There are a couple of exceptions: UnmanagedCallbackHandlers is used solely for unmanaged interop; BinaryReader.Frame and BinaryWriter.Frame are private structs and are performance-sensitive.

Override equals and operator equals on value types (15 issues), Structures should be immutable (7 issues)

Again, very good rules. All public structs must follow it. Found issues are related to private structs, which have clearly defined use cases and are not used in comparisons and data structures.

Base class should not use derivatives (3 issues)

Open/closed principle in action. 2 public classes violate this rule: TcpDiscoveryIpFinderBase and EvictionPolicyBase. These classes are explicitly closed for modification from outside of the assembly, constructors are internal. Only predefined derivatives are supported by the API.

Property getters should be immutable (2 issues)

Relates to Choosing Between Properties and Methods MSDN guidelines. Main exception here is lazy initialization (TransactionsImpl.Current). The other issue is ClusterGroupImpl.TopologyVersion, which performs expensive RefreshNodes() nodes operation and should really be a method: IGNITE-6370.

Conclusion

We have explored a small part of issues found by NDepend, and already filed 3 tickets. The rule set is incredibly rich and covers many guidelines and best practices. I can recommend the tool for project of any size.

As for Ignite.NET, we have decided to integrate NDepend with our CI pipeline: there is excellent TeamCity support.