Analysing Ignite.NET Code With NDepend
Along with unit testing, continuous integration, and code review, static code analysis is invaluable for maintaining healthy code base.
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.
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 (
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
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
Task: IGNITE-6231. Exception handling is also missing in that thread, which can potentially bring down entire process.
Moving on to
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.
IgniteJniNativeMethods class is
internal static, so this looks like a false positive to me.
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;
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:
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.
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.