Refactoring insight: simple foreign key cleanup or a domain service?

On my quest for DDD/TDD nirvana, I’m going to start documenting moments of insight on real projects when design problems are solved, principles suddenly make sense for the first time, or pitfalls are avoided. I want to do it partly for posterity, and partly to help others who are also learning. Here’s the first one.

One of my projects at the moment is developing a simple asset management system. It’s basically just a catalog of assets (computers, devices and office furniture) with various details (specs, serial numbers, notes on wear and tear etc). Each asset may be issued to a person.

I was writing some tests for my concrete repositories (mainly in order to check the database mappings are correct), when the question arose: what should happen to a person’s assets when the person is deleted?

My first thought was simply to edit the mapping XML and change a cascade option so Asset.AssignedToPersonID would be set back to NULL. However, I realised this is more than just a foreign key issue for NHibernate — returning a person’s assets when they leave the organisation is a significant domain concern.

If I take the shortcut and leave it implemented as a cascade option:

  • It’ll work fine, but the business rule will be implicit, rather than explicitly expressed somewhere as something that happens when a person is terminated. Other developers probably won’t even notice it at all.
  • The implementation won’t be testable without hitting a live SQL instance.
  • The business rule won’t be observable in other test cases unless they hit a live SQL instance, because mock repositories will likely not factor for it.
  • If we ever move away from NHibernate, or otherwise need to recreate the database mappings, this feature could be lost, and will probably only be rediscovered after FK constraint errors arise.

That’s no good. Let’s make this business rule an official part of the domain model instead, with a fully-fledged domain service:

public interface IPersonTerminatorService
{
    void TerminatePerson(Person person);
}

public class PersonTerminatorService : IPersonTerminatorService
{
    IPersonRepository personRepository;
    IAssetRepository assetRepository;

    public PersonTerminatorService(IPersonRepository personRepository,
        IAssetRepository assetRepository)
    {
        DesignByContract.Require(personRepository != null, 
            "personRepository cannot be null.");
        DesignByContract.Require(assetRepository != null,
            "assetRepository cannot be null.");
        
        this.personRepository = personRepository;
        this.assetRepository = assetRepository;
    }

    public void TerminatePerson(Person person)
    {
        ReturnAllAssetsIssuedToPerson(person);
        this.personRepository.Remove(person);
    }

    public void ReturnAllAssetsIssuedToPerson(Person person)
    {
        IEnumerable<Asset> assets = 
            this.assetRepository.GetAssetsIssuedToPerson(person);

        foreach (Asset asset in assets)
        {
            asset.Return();
            this.assetRepository.Save(asset);
        }
    }
}

The implementation is not tested yet (and will likely be refactored later because I think it does too much stuff) but here’s what it’s going to satisfy:

[Test]
public void Terminating_a_person_returns_all_assets_issued_to_them()
{
    Asset a = new Computer() { Name = "Apple Powerbook" };
    Asset b = new Computer() { Name = "Dell Dimension" };
    Asset c = new Computer() { Name = "IBM ThinkPad" };

    Person p = new Person("Richard");

    a.IssueTo(p);
    b.IssueTo(p);
    c.IssueTo(p);

    IPersonTerminatorService terminatorService =
        ServiceLocator.Current.GetInstance<IPersonTerminatorService>();

    terminatorService.TerminatePerson(p);

    Assert.IsNull(a.IssuedTo);
    Assert.IsNull(b.IssuedTo);
    Assert.IsNull(c.IssuedTo);
}

It’s a start!