NHibernate 2.0: Changes Overview

My post .NET Framework 3.5 SP1: Changes Overview on analysis evolution, structure and quality of the .NET framework code base with NDepend became popular. It shows the interest of the community for the under the hood of popular Fx. Thus came the idea of publishing a similar post for the new release of NHibernate 2.0. This post NHibernate 2.0 Gold Release analysis NHibernate 2.0 with NDepend and lists breaking functional changes, while I’ll focus more on structural changes.

Changes Overview (compared to NHibernate v1.2.1)

# IL instructions    176 818 to 245 106      (+68 288   +38.6%)
# lines of code (LOC)    25 960 to 36 143      (+10 183   +39.2%)
# lines of comment    25 135 to 29 401      (+4 266   +17%)
Percentage Comment    49% to 44%      (-5%)
# Assemblies    1
# Namespaces    46 to 65      (+19   +41.3%)
# Types    806 to 1 262      (+456   +56.6%)
# Methods    8 190 to 12 016      (+3 826   +46.7%)
# Fields    2 166 to 3 842      (+1 676   +77.4%)

4.852 new
public methods :

SELECT METHODS WHERE IsPublic AND WasAdded

548 new
public types :

SELECT TYPES WHERE IsPublic AND WasAdded

21 new mamespaces :

SELECT NAMESPACES WHERE WasAdded

2 namespaces removed :

SELECT NAMESPACES WHERE WasRemoved

127 public types removed :

SELECT TYPES WHERE IsPublic AND WasRemoved

5 non-public methods became public:

SELECT METHODS WHERE IsPublic AND VisibilityWasChanged AND IsInNewerBuild

1.230 methods
where code was changed :

SELECT METHODS WHERE CodeWasChanged

380 types
where code was changed :

SELECT TYPES WHERE CodeWasChanged

The following treemap/metric view shows the impact of methods changes or added (in blue). It clearly looks like that the entire code base has been refactored:

Assembly dependencies

The assembly structure hasn’t changed. The NHibernate code is still packaged within one assembly named NHibernate.dll, and this is IMHO the best possible choice since there is no reason to have several physical components for the NHibernate Fx.

NHibernate.dll is almost using the same set assemblies: Castle.Core.dll v1.0.3.0 has been added and also Castle.DynamicProxy2.dll v2.0.3.0 is used in lieu of Castle.DynamicProxy.dll v1.1.5.0. Here is the simple graph of assemblies dependencies.

Internal namespaces dependencies

As noted by Jim Bolla a few months ago in his post Analyzing NHibernate with NDepend, the NHibernate code is quite entangled and unfortunately this problem hasn’t been fixed for NHibernate v2.0. Basically, on 65 namespaces, 63 namespaces depend directly or indirectly on 62 other namespaces. This is what is shown by the big black square in the dependencies matrix below, taken with the indirect dependencies option of NDepend. A black cell means that the 2 corresponding namespaces are involved in a dependency cycle of minimal length the number displayed.

IMHO, the NHibernate team should fix this problem asap. This article Controlling Dependencies to get Clear Architecture explains how we get rid for good of spaghetti in the NDepend code base within in a few days, about 2 years ago. Retrospectively, I estimate that this is by far the best Return On Investment on quality we ever done, much better that any of the thousands automatic tests we wrote. As explained in the article, we now use some NDepend capabilities to prevent new cycle to appear.

The picture below shows direct dependencies between namespaces of NHibernate. The legend is:

A blue cell means: {the X Namespace} is using {the Y Namespace}.
Weight of a blue cell means: W members (methods and fields) of the {the X Namespace} are used by {the Y Namespace}.
A green cell means: {the Y Namespace} is used by {the X Namespace}.
Weight of a green cell means: W methods of the {the Y Namespace} are using {the X Namespace}.
A black cell means: {the X Namespace} and {the Y Namespace} are using each others.
A red tick on a cell means: the coupling has been changed.
A red tick with a plus on a cell means: the dependency has been created.
A red tick with a minus on a cell means: the dependency has been removed.
A Namespace name underlined means that its code has been changed.
A Namespace name in bold means that it has been added.

Code quality

According to the following CQL rules, 464 methods are good candidate to be refactored because they each violates one of the metric threshold.

// <Name>Quick summary of methods to refactor</Name>

WARN IF Count > 0 IN SELECT METHODS WHERE 
                                           // Metrics' definitions
     (  NbLinesOfCode > 30 OR              // http://www.ndepend.com/Metrics.aspx#NbLinesOfCode
        NbILInstructions > 200 OR          // http://www.ndepend.com/Metrics.aspx#NbILInstructions
        CyclomaticComplexity > 20 OR       // http://www.ndepend.com/Metrics.aspx#CC
        ILCyclomaticComplexity > 50 OR     // http://www.ndepend.com/Metrics.aspx#ILCC
        ILNestingDepth > 4 OR              // http://www.ndepend.com/Metrics.aspx#ILNestingDepth
        NbParameters > 5 OR                // http://www.ndepend.com/Metrics.aspx#NbParameters
        NbVariables > 8 OR                 // http://www.ndepend.com/Metrics.aspx#NbVariables
        NbOverloads > 6 )                  // http://www.ndepend.com/Metrics.aspx#NbOverloads

This stance on metrics is a bit extremist and honestly, we have a bunch of methods in the NDepend code base that still don’t abide by all these thresholds. While abiding by metric threshold helps a lot to have maintainable code, my opinion is that quality effort must be focused first on rationalizing componentization and dependencies, with non-cyclic dependencies between components, low level components and logical component (namespaces) instead of physical components (assemblies).

However, there is a fundamental metric not represented here: Test Coverage. I tried to gather Test Coverage metrics for NHibernate 2.0 to make this data analyzed by NDepend. Unfortunately just running the NHibernate.Test-2.0 project with the option Run Test(s) of TestDriven.NET lead to: 427 passed, 755 failed, 14 skipped, took 3738,55 seconds. I suspect I don’t have all DB stuff installed (I am not really a DB guy) and am a bit lazy to go thought all the operation to make it work. (Btw, any code coverage file produced by NCover or VSTS Coverage on NHibernate 2.0 is welcome, just send it with http://www.transferbigfiles.com/ and let me know the url where I can download it. My email is psmacchia at google mail. This way I could complete this post with obtained results).

Conclusion

This awesome release of NHibernate is the result of a lot of work done by extremely talented guys. As said, I am not a DB guy, I cannot appreciate the real value of this framework. However, googling it a few seconds lets find dozens of thousands of enthusiast users.

This post focuses on structure, quality and evolution and thanks to NDepend capabilities, I find some good things but also some important room for improvement (IMHO) within a few minutes that I wanted to share.

Fabio Maulo

@Patrick
Analyze dependency from classes included inside .NET FW is not necessary and less when you are talking about dialects because all dialects can be extracted from code-base without any problems.

Compare NH1.2 with NH2.0 is unnecessary if you know the history of NH. NH2.0 was a big work and many classes was completely rewritten.

Dependency from castle: 4 hours to remove it completely.

I don’t understand if your effort is oriented to have a better NH or to publicize NDepend using NH. If you have a patch to remove some “dependency” remember that we are happy to analyze your proposals.
Patrick Smacchia

Jack, after verification:

the method
NHibernate.Driver.OracleClientDriver.CreateConnection()
creates a
System.Data.OracleClient.OracleConnection..ctor()

and the method
NHibernate.Driver.OracleClientDriver.CreateCommand()
creates a
System.Data.OracleClient.OracleCommand..ctor()

hence the dependency on System.Data.OracleClient.

With your ‘WTF’ you point exactly why a tool such as NDepend is essential: the code base structure is often different than what we think it is.
Jack

WTF???? Why is there a dependency on System.Data.OracleClient?????
Tuna Toksoz

Patrick, you need to create a database named nhibernate. IThen rerun the test again. It took
3 mins 50 secs 58 msecs to run all the tests in the trunk(and I assume it will take less for the release) and I had 1407 passed 5 failed 38 ignored. 5 failing tests were dialect issues complaining This fixture applies to Firebird, etc.
http://www.NDepend.com Patrick Smacchia

Brian, yes more than one hour, no typo problem, I think that tests tried and tried to create unsuccessfully some DB, but that is my own theory.

Damon, yes there are many (many) things in the pipe but cannot talk about it so far.
Brian Johnston

Is that 3738.55 seconds to run unit tests (1 hour, 2 minutes) a typo or am I doing my math wrong? That can’t be right.

Seems extremely excessive for what appears to be only about 1200 tests if I did my addition right (427 passed, 755 failed, 14 skipped)
Brian Johnston

This is good information. Thank-you for doing this.

It does concern me that a framework traditionally used hand in hand with other frameworks to help reduce dependencies (Google ‘NHibernate’ and ‘Dependency Injection’ to see what I mean) has so many cross dependencies itself – not good advertisement in my mind.

I think this shows the problem we all face when writing software – when is it time to cut the cord to the past and do a true ‘new’ version? By trying to encompass all things, we fall apart at the seams in many cases, so sometimes the best thing to do is limit what we try to do and not fall into the jack of all trades, master of none in the ‘code’ sense of the saying.

We all know NHibernate is powerful in the hands of the right developer who knows how to utilize it correctly, but perhaps a rewrite and decoupling of the past would result in a better framework.

Just my opinion.
http://blog.domaindotnet.com Damon Wilder Carr

As always fantastic. Indeed your SP1 post subconsciously ‘gave us permission’ to add the NDepend analysis.

It just happened, we didn’t even think about it to be honest.

We had done this analysis from the earliest bits of the alpha and like most software we use/construct/care about, these artifacts are expected, not optional for us.

CANNOT WAIT for the new version of NDepend.

Actually I am sure you have thought of this, but any plans for ‘Linq to CQL’? That would be utterly awesome….
http://www.codinginstinct.com Torkel

This was really interesting, very useful to see detailed stats on changes in assembly versions.

I have studied the nhibernate source and find it very nicely structured and coded, there are of course many ways it could be improved. Stats like this is a really good starting point
http://www.NDepend.com Patrick Smacchia

Will, I indeed just got this feedback from some others NHibernate developers.

I hope that your team and the Hibernate team will find a way to improve the overall architecture because with such entangling, it must not be fun everyday to develop it.
http://primedigit.com/ Will Shaver

One of the difficulties in developing features for NHibernate and with performing code cleanups is the desire to keep it somewhat in sync with it’s older brother Hibernate. There’s a constant and necessary struggle between wanting to improve the core and wanting to keep it as true to Hibernate as possible. While I haven’t tried to reduce related complexities I imagine it would cause a rather large deviation from Hibernate. Perhaps we can try and get Hibernate to refactor some and then can use those changes as well.

This information is quite interesting. Thanks for providing it.

-Will (Just barely a nhibernate developer.)

NHibernate 2.0: Changes Overview

Archives

Friends of CodeBetter.Com