NHibernate Code Base Analysis

Posted by Patrick Smacchia on November 30, 2010

On the NHibernate leaders request, I had an opportunity to review/audit the brand new NH v3.0.0 code base. My analysis is based on facts obtained through the new reporting capabilities of NDepend. The analysis is published here on the NH official blog and here is the NDepend report on NH.

What I tried was not to just show what is good or wrong
in my opinion, but also to explain why I think it is good or wrong.

For a brief  summary, NH is ok on:

  • The too many assemblies problem (my god, 95% of professional projects I can see have hundreds
    of asm, without any valid reason except to dramatically slow down everything on the
    developers workstation and build machines!!!)
  • Not enough code coverage, NH has a healthy global 76% code coverage ratio. I mean, just NH handcrafted and generated code, not counting test code. Btw, around 18K LoC are generated on a total of 63K LoC.
  • API breaking change, the team is pretty careful with the NH public surface and this attention is made obvious by comparing NH V3.0.0 with the previous NH v2.1.2 version.
  • Dead code

I found room for improvement in:

  • Code contract usage, NH doesn’t use any code contracts Fx (nor MS Code Contract API nor Debug.Assert(…)).
  • Despite the global great 76% code coverage ratio, many methods or types are 90%+ covered by tests instead of 100%. The problem is that often, subtle bugs hide in the remaining few percents hard to test and not covered code.
  • A better global code architecture (this is not feasible since NH public API cannot be refactored for obvious reasons)
  • A respect for basic code metrics quality thresholds (too complex methods, god classes, fat code, methods with too many parameters etc…).
This entry was posted in Uncategorized. Bookmark the permalink. Follow any comments here with the RSS feed for this post.
  • http://codeclimber.net.nz Simone Chiaretta

    I think most of these “uglyness” comes from the fact that NH is a port from Java.
    I guess that if you analyze Lucene.Net it will be the same if not worse…

    But that’s the cost of porting apps in a semi-automated fashion between languages

  • http://www.simonrhart.com Simon Hart

    Interesting post Patrick.

    I’m not sure about the “too many assemblies problem”. Having more assemblies (in enterprise apps) allows you to completly decouple your application so that a single module can be deployed for an update for example just like we used to be able to do in the native days.

    Since the uptake of statically-linked technologies like .NET, many apps are so tightly coupled and code is so reused that it is impossible to separate it. Look at NServiceBus, I think there are some 20 or 30 assemblies in that project.

    I quite like code contracts from Microsoft Research, but this is fairly new. Are you saying there is no home-rolled code contracts in NHibernate?

    I’m also not too fussed about 100% code coverage. The real important thing for me is single responsibility principle, what that you’ll never see god classes, without it, you will see pain at some point.

    Keep up the good work on NDepend.

    Simon Hart [MVP]

  • http://codebetter.com/members/Patrick-Smacchia/default.aspx Patrick Smacchia

    Simon, from what I saw, developers are not using hundreds of assemblies with the single module fix deployment in mind. They are using many assemblies to get componentization, code organization and structuring, and end up being stuck with all the problems mentioned due to the physical nature of assembly. Today again, a user asked about a 170 K.LoC application spawned on 230 assemblies!

    >Are you saying there is no home-rolled code contracts in NHibernate?

    There is thousands of unit tests, but I haven’t seen contracts, i.e checks in code that fail abruptly when broken. The technology doesn’t matter, I use Debug.Assert(…) to implement contracts with success for ages.

    >I’m also not too fussed about 100% code coverage.

    Then please try it for a while, and hopefully, you’ll see a large number of flaws and bugs discovered early in code that you are not usually testing because it is hard to tests. (Hard to test == error prone)

    >The real important thing for me is single responsibility principle, what that you’ll never see god classes, without it, you will see pain at some point.

    Sure, but a responsibility is not something concrete, that can be checked automatically by a tool. In the NH analysis post I prone to avoid large efferent coupling for classes, something that can be checked, in order to avoid god classes.

  • Phillip

    >API breaking change

    I don’t see any issue with API breaking changes on major releases, 2.1 -> 3.0. As long as it’s known well in advance. “Hey look guys, to help improve NH we will be re-doing the API which means 3.0 will not be backwards compatible with 2.1″

    Thats my opinion tho.