Coding out of procrastination

NHibernate supports several programming paradigms that allows you to write what I consider good code, some of the ideas expressed in this post are opinionated and the merit of such should be reviewed against your own coding preferences and applicability to your project.

Creation

NHibernate requires each mapped type to have a parameterless constructor – However, this parameterless constructor does not need to pollute your domain model as it can be hidden from external code by marking it with protected or protected internal whilst your parameterized constructors can be public , meaning you can still enforce integrity at the constructor level, thus never allowing for creation of invalid entities. If for some reason you never want to create a Dog without a date of birth you could enforce this whilst still using NHibernate by doing something as simple as this:

public class Dog
{
    protected internal  Dog() { }

    public Dog(DateTime dateOfBirth)
    {
        this.dateOfBirth = dateOfBirth;
    }

    private DateTime dateOfBirth;
    public DateTime DateOfBirth
    {
        get
        {
            return dateOfBirth;
        }
        protected set
        {
            dateOfBirth = value;
        }
    }
}

(You could optionally use an automatic property if using C# 3.0)

Where you don’t require a separate factory I’ve personally taken to creation static methods to give some more semantic meaning to the creation. Here’s a trivial example, but this style really comes into its own when you have multiple constructor parameter sets that have the same signature, not only do the static method names gives meaning, but you can’t vary a constructor unless you vary its parameter set too.

        public static Dog CreateFromEstimatedAge(int estimatedAge)
        {
            var dog = new Dog {isEstimated = false,
                               dateOfBirth = DateTime.Now.AddYears(-estimatedAge)};
            return dog;
        }

2) Encapsulation

Classes that contain nothing but public virtual type name { get; set; } are potentially a big code smell.  To preserve encapsulation one needs to carefully consider the visibility of each member and any constraints that should be imposed on the setters.  For example, if you had a database key that was a simple auto-increment identity that required no visibility then you have no good reason to define a public setter for this property. On brownfield projects you should also consider if you need to map everything that’s in the database, if you’ve already gone past that point, use either the inbuilt tools or Resharper to find usage of properties (or rather where no usage exists).

2b) Don’t expose IList

These are big smells as far as I’m considered, when you do something like person.Children.Add(new Child("Fabio")) you’re violating the "tell, don’t ask principle", you should be delegating the work of adding a child to the person, however subtly in this case you’re asking for the collection of children and doing the work yourself.  You should expose methods "AddChild()" – which throws it own meaningful exception when constraints are violated, and return an IEnumerable of Children via the .Children property (getter only). I disagree with throwing a not implemented via your own collection or not supported via a read only list since where a class implements a particular interface it should be substitutable without worry, I shouldn’t have to know that underneath it is a read only collection or a list stole of its owed behaviour.

Most consumers of an IList I’ve seen in projects use only the iterator and the Count property – which is available via Linq extensions method of Count() – perhaps not as efficient but I’m willing to make this tiny sacrifice. Linq extensions method also provide numerous other operations over IEnumerables including sorting, selection, projection etc.. Expose the lowest common interface of IEnumerable and you’ll be able to change the underlying concrete implementation as you please.  Avoid creating your own collection type to handle constraints as this adds extra work and in my experience quite often the constraints that govern the collection are owned and unique to the particular type of parent.

4) Validation

You should be throwing a suitable exception if immediately possible when a call to a setter is attempting to put the object into an invalid state. So person.Age = -5 should throw ArgumentException. If a string property can not be null or empty, throw an ArgumentNullException when null and an ArgumentException when empty, follow the BCL conventions.  Where more complex validation is carried out and it is the responsibility of the object to perform this validation then move away from properties, where properties run for more than a few lines or make calls to foreign objects (exception of lazy loading) then its time to consider moving them to methods.

I’ve seen a couple of people object to immediate validation, preferring to allow consumers to push the object into a known invalid state and then call IsValid or Validate or something similar (and even some delegating this to Interceptors etc..), but you can still achieve summary/delayed validation whilst not weakening your domain integrity which I’ll demonstrate in another blog post in a few days.

5) Repository

There’s been a lot of talk on generic repositories recently, I’m definitely a fan of hiding the generic repository in an inner repository, exposing find all by criteria style methods on the domain repository gives away too much of its own responsibility as well probably leaking some ORM specific dependencies out of your repository (think DetachedCriteria), and even NHibernate.ObjectNotFoundException etc.. This style of repository I think works quite well:

using System;

namespace Cruisers.Ecommerce.Domain.Repository
{
  public interface ICruiseRepository
  {
      IEnumerable<ICruise> GetCruisesOnDate(DateTime date);
      IEnumerable<ICruise> GetCruisesThatAreDiscounted();
      ICruise GetCruiseById(int id);
      ICruise GetCruiseByBrokerCodeName(string codeName);
  }
}

namespace Cruises.Ecommerce.DataAccess
{
    public class NHCruiseRepository : ICruiseRepository
    {
        private readonly IDaoRepository<Cruise> daoRepository;

        public NHCruiseRepository(IDaoRepository<Cruise> daoRepository)
        {
            this.daoRepository = daoRepository;
        }

        public IEnumerable<ICruise> GetCruisesOnDate(DateTime date)
        {
            throw new System.NotImplementedException();
        }

        public IEnumerable<ICruise> GetCruisesThatAreDiscounted()
        {
            throw new System.NotImplementedException();
        }

        public ICruise GetCruiseById(int id)
        {
            try
            {
               return daoRepository.Get(id);
            } catch(NHibernate.ObjectNotFoundException nhException)
            {
               // throw new ArgumentOutOfRangeException("id","cruise not found",nhException);
                throw new CruiseNotFoundException("cruise with id " + id + " not found.",nhException);
            }
        }

        public ICruise GetCruiseByCodeName(string codeName)
        {
            throw new System.NotImplementedException();
        }
    }

}

namespace Cruisers.Common
{
    public interface IDaoRepository<T, K>
    {
        T Get(K id);
        IList<T> FindAll(DetachedCritiera critiera);
    }
}

But eventually you may reach a point when you have to consider sorting, paging, etc.. in these cases one can create a Query object to nicely encapsulate sorting and paging options and provide methods to accept such e.g.

IEnumerable<ICruise> GetCruisesOnDate(OnDateQuery onDateQuery)

As a further improvement in 2.1 we’ll be able to elegantly use the specification pattern (you can today but it is some work) and have the NH repository convert the predicates to NH queries, I intend to spike an example from the trunk soon. NH 2.1 will give us the ability to use Linq natively, but I recommend you avoid exposing this as you’ll have query logic (admittedly NH clean) thrown throughout your application, instead we harness the forthcoming Linq power to use clean specifications instead.