One of the projects I’m on has over 2,200 stored procedures. Until I arrived, none of these were under source control — the ‘stable’ version was whatever was on the production database server, and the ‘trunk’ version was whatever was in the development database server (which incidentally wasn’t even backed up).

Anyway, I set about exporting all the stored procedures and checking them into source control. But as easy as it sounds, it actually took a few goes to get the it all right. You see, when generating a CREATE script with IF NOT EXISTS, SQL Management Studio has a habit of generating entire scripts as one giant EXEC sp_executesql string instead of just putting a GO in between. This isn’t very good for development — you don’t get intellisense, syntax highlighting, etc inside a string.

To get around this problem, I ended up generating two separate scripts — first a DROP (with IF NOT EXISTS), then a CREATE (without it) — and set SSMS to put each in the same file (Append to File). This gave me one script per stored procedure, each with an IF EXISTS DROP, GO then CREATE statement — perfect for source control.

Here are some other tips for configuring SQL Management Studio (via Options > SQL Server Object Explorer > Scripting) to make it automatically generate friendly scripts:

• Disable Include descriptive headers. They sound helpful, but all they do is add the script date and repeat the object’s name. This is really annoying because it clutters up my results for source-code text search in Visual Studio.
• Disable Script USE <database>. We often have several copies of the same database running with different names on the same SQL box (e.g. for different development branches). Hard-coding the name of the database in all your scripts means you have to manually change all the USE statements every time you run them. And trust me — the risk of having your changes leak over and get applied to the wrong database is not really something you want to be worrying about.
• Enable Include IF NOT EXISTS clause. Stored procedures, triggers, functions — it’s much easier to just nuke and re-create them every time instead of worrying about the semantics of CREATE vs ALTER. This also means you can just run each script over and over and over again.

If you get it right, SQL Management Studio 2008’s Script DROP and CREATE should produce something that looks like:

IF  EXISTS (SELECT * FROM sys.objects WHERE object_id = OBJECT_ID(N'[dbo].[usp_Employee_Get]') AND type in (N'P', N'PC'))DROP PROCEDURE [dbo].[usp_Employee_Get]GOCREATE PROCEDURE [dbo].[usp_Employee_Get]AS        SELECT                EmployeeID,                FirstName,                LastName        FROM                EmployeeGO

We also put permissions (e.g. GRANT EXEC) at the end of each script, so they are restored each time the stored procedure is created.

Just before Christmas we had a problem with an application database where rows were being mysteriously deleted. Unfortunately the table in question is written to by 20-30 stored procedures, so there was no single point in the application where I could put a trace.

Instead, I wrote a quick generic script that would automatically create the necessary triggers to log all INSERT, UPDATE and DELETE commands on a table of your choosing (or just generate the script).

For example, every time someone modified data in the HR.Employees table, a row would be written to HR._Employees_Audit with all the new values, time stamp, command, and name of the user who did it.

You can download it here: sql-server-auto-audit.sql

This insight follows on from an earlier article where I identified that the business rule “when an employee leaves the organisation, all their assets must be returned” was part of the domain model, and not just a matter of cleaning up foreign keys in the database.

I originally implemented this rule as part of a dedicated domain service called IPersonTerminatorService. Later on, I refactored it out into a standalone IAssetReturnerService to satisfy the single-responsibility principle.

But this morning I realised something: in real life, it’s the employee himself who returns all his stuff — not some other process. By taking this responsibility away from the Employee class I’ve committed two sins: creating a fictional concept in the domain, and reducing the capabilities of the Employee (leading to an anemic data-only domain model).

Time to give this responsibility back to the employee:

Here’s a snippet from my prototype NHibernateRepository<T> I mentioned the other day. Can you see what I’ve done wrong here?

public abstract class NHibernateRepository<T>{    protected ISession unitOfWork;    protected IEntityValidator<T> validator;    protected NHibernateRepository(ISession unitOfWork,        IEntityValidator<T> validator)    {        if (unitOfWork == null)            throw new ArgumentNullException("unitOfWork");        if (validator == null)            throw new ArgumentNullException("validator");        this.validator = validator;        this.unitOfWork = unitOfWork;    }        protected virtual void Validate(T item)    {        IEnumerable<RuleViolation> ruleViolations =             this.validator.GetAllRuleViolations(item);        if (ruleViolations.Count() == 0)            return;        string message = String.Format(            "The {0} '{1}' could not be persisted because it failed validation.",            typeof(T).Name, item);        throw new RuleViolationException(message, ruleViolations);    }        public virtual void Save(T item)    {        Validate(item);        using (ITransaction transaction = this.unitOfWork.BeginTransaction())        {            this.unitOfWork.SaveOrUpdate(item);            transaction.Commit();        }    }    // ...}

See that Validate() method? It’s very clever, automatically checking entities are valid before it commits them to the database. Unfortunately, validating entities is not the responsibility of a repository. This is a big fat violation of the single responsibility principle (SRP), and should be moved to a higher-level domain service instead.

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!

Currently I am responsible for maintaining a big legacy .NET ERP system. It’s plagued by something I like to call cargo-cult commenting — source code comments that are ritually included, for some reason, but serve no real purpose at all. These are the sort of comments that:

• Are written for aesthetic reasons more than anything else
• Usually just repeat the next line of C# in sentence form
• Tell you what the code does, not the reasons why
• Don’t add any value whatsoever
• Just make the code base even more bloated and crappy than it was before

Here are a few examples to demonstrate exactly what I mean.

/// <summary>/// POST/// </summary>/// <param name="oSpace"></param>public override void POST(ref Space oSpace)

//Create a new attribute.XmlAttribute newAttr = xmlDocuments.CreateAttribute("Directory");newAttr.Value = "CompanyCorrespondenceDocs";

// read-only?if (bVehicleEdit == false) oField.SetReadWrite(ref oSpace, true);

// return the vehicle riskreturn DataAccess.ExecuteXml("Result", "Row", "Vehicle_VehicleRisk_Get",     iVehicleID).SelectSingleNode("Result/Row");

This sort of rubbish gets in the system because people are taught the stupid rule that comments are always good, no matter what their purpose is. One developer sees another developer writing a line of comments for every 2-3 lines of code, copies it, and pretty soon sections without any green look naked and unfinished. If only unit tests were written like this!

Anyway, my golden rules for code commenting are:

1. There are only two valid reasons for writing comments — to explain why code was written (e.g. original business motivation), or as a warning when something unexpected happens (here be dragons).
2. Write the reasons why something is done, not how.
3. If you find yourself putting comments around little blocks of 3-6 lines of related code, each of these blocks should probably be a separate method with a descriptive name instead.
4. If you can’t write anything useful (i.e. that couldn’t be determined by a quick glance at the code), don’t write anything at all.
5. BUT if you find yourself regularly needing to write comments because it is not clear what’s going on from the code, then your code probably isn’t expressive enough.

Writing good comments is hard. One habit I’ve recently adopted is to paste entire paragraphs of relevant user requirement into my code to explain why it was written. It’s working really well — it saves me time, and produces comments of very high quality, straight from the horse’s mouth. I encourage you to try it too.