Analyzing Ignite.NET Code With NDepend
Analyzing Ignite.NET Code With NDepend
See how NDepend works and how you can use it for code analysis to make sure your Ignite.NET code is clean and providing the best performance possible.
Join the DZone community and get the full member experience.Join For Free
Along with unit testing, continuous integration, and code review, static code analysis is invaluable for maintaining a healthy code base.
Ignite.NET uses FxCop and ReSharper code analysis from th early days, and this process is included in continuous integration on Ignite TeamCity, so I was fairly confident that our code is (mostly) fine.
Getting Started With NDepend
We are going to work only with the
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:
2,654 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 the red "4" 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 a 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: the 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”). The 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 the 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 kind of a “thing in itself” which can be moved anywhere, extracted to an external assembly, etc. This makes refactoring easier.
Such decoupling can be achieved by introducing additional interfaces. In the real world, however, introducing an interface just for the sake of decoupling within a single assembly is not feasible. Most of the core Ignite functionality is quite tightly coupled and is not expected to exist separately.
A good example of this is the 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)
The open/closed principle in action. Two 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. The main exception here is lazy initialization (
TransactionsImpl.Current). The other issue is
ClusterGroupImpl.TopologyVersion, which performs the expensive
RefreshNodes() nodes operation and should really be a method: IGNITE-6370.
We have explored a small part of the 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 projects of any size.
Published at DZone with permission of Pavel Tupitsyn . See the original article here.
Opinions expressed by DZone contributors are their own.