Today when preparing for our code review I happened upon a few methods in our base repository. Those methods were using NHibernate to retrieve collections of objects from persistence. I started scanning over the methods and figuring out what they were trying to accomplish. After understanding their function I noticed that they were prime candidates for a refactoring.

The methods were:

public virtual IList<Entity> GetAll()
{
	return Session.CreateCriteria(typeof (Entity)).List<Entity>();
}

public virtual IList<Entity> GetActiveItems()
{
	ICriteria criteria = Session
		.CreateCriteria(typeof(Entity))
		.Add(Restrictions.IsNull("InactiveDate"));

	return criteria.List<Entity>();
}

public virtual IList<Entity> GetItemsLikeName(string name, string columnToCompareBy)
{
	ICriteria criteria = Session.CreateCriteria(typeof(Entity))
		.Add(Restrictions.IsNull("InactiveDate"))
		.Add(Restrictions.Like(columnToCompareBy, name, MatchMode.Start).IgnoreCase());

	return criteria.List<Entity>();
}

Analysis:

Now the first thing that tipped me off to the need for Refactoring was the DRY (Don’t pete and repete yourself) principle, which can be extrapolated into Attwood’s Curly’s Law and even further to the Single Responsibility Principle. At least, I thinks DRY’s a principle, maybe it’s classified as a technique, or to be more in tune with coding hype a code smell. Lol, code smells, it may work if compared to flatulence but if you compare it to cheese, the more pungent the odor, the better the cheese. Actually it has more to do with identifying different smells so that you know what needs to be refactored or reworked entirely. Anyway, they all pretty much are talking about the same thing, minimizing duplicate code/rules/processes.

The Break Down:

The first thing I noticed was the responsibility of all the methods, they are responsible for retrieving a collection of objects. I observed this by the methods’ return types of IList<Entity> and their names, GetAll(), GetActiveItems(),GetItemsLikeName().

Next all of the methods are following the same basic pattern(process) of creating an ICriteria object, setting the specified ICriterion(where my object equals some value) on that ICriteria and finally executing the retrieval of the matching objects. The following is some psuedo code of that pattern(process)

ICriteria criteria = Session.CreateCriteria(typeof(Entity))
		.Add(ICriterion)
return criteria.RetrieveMatchingObjects<Entity>;

The Refactor:

Now we know the reponsibility and the process of all three methods. From this knowledge we can refactor those methods to use one method, but we will want to keep the signatures of each method, because they still have a separate responsibility as well. Let’s see the common concern is collection retrieval through different criteria. To address handling the different criteria we can utilize c#’s keyword params to pass in a variable number of criterion. Now, our combined method signature should look like:

protected virtual IList<Entity> GetAllItems(params ICriterion[] criterion)

Our refactored method is ready to return a collection of objects and to accept n number of ICriterion.

Next we need to create our ICriteria creation:

ICriteria getCriteria = Session.CreateCriteria(typeof(Entity));

After that it’s on to handling our array of ICriterion:

if (criterion != null)
{
	foreach (var criteria in criterion)
	{
		getCriteria.Add(criteria);
	}
}

First we need to check if the array is null, it is always good to check for null before trying to loop through an IEnumrable. Once we are sure that the array is not null we can loop through that bad boy and add the criterion to our ICriteria object. At this point we can execute our query, I prefer burning at the stake, but it’s up to you. Anyway our retrieval and return will (survey says) look like:

IList<Entity> result = getCriteria.List<Entity>();

Next on the proverbial platter, both the GetActiveItems() and GetItemsLikeName() methods are using the same criteria in their queries, the null check of Inactive Date. Ah ha, another point to refactor, stop the repetition. Let’s create a read onlyICriterion property named WhereInactiveDateIsNull, remember fluent interfaces are your friend. If the name is unclear now, it may make more sense in a bit, so chill.

protected virtual ICriterion WhereInactiveDateIsNull
{
	get
	{
		return Restrictions.IsNull("InactiveDate");
	}
}

Alright so we now have our one method to rule them all, or at least the three methods we were looking at before.

The Result:

So now all of our methods are going to call the method we created and tell that method their own specific responsibilites through the criterion they pass in.

protected virtual ICriterion WhereInactiveDateIsNull
{
	get
	{
		return Restrictions.IsNull("InactiveDate");
	}
}

protected virtual IList<Entity> GetAllItems(params ICriterion[] criterion)
{
	ICriteria getCriteria = Session.CreateCriteria(typeof(Entity));

	if (criterion != null)
	{
		foreach (var criteria in criterion)
		{
			getCriteria.Add(criteria);
		}
	}

	return getCriteria.List<Entity>();
}

public virtual IList<Entity> GetAll()
{
	return GetAllItems();
}

public virtual IList<Entity> GetActiveItems()
{
	return GetAllItems(WhereInactiveDateIsNull);
}

public virtual IList<Entity> GetItemsLikeName(string name, string columnToCompareBy)
{
	ICriterion whereColumnValueStartsWithNameIgnoringCase
		= Restrictions.Like(columnToCompareBy, name, MatchMode.Start).IgnoreCase();

       	return GetAllItems
	(
		WhereInactiveDateIsNull,
		whereColumnValueStartsWithNameIgnoringCase
	);
}

The Interlude:

“Oh, this little guy? I wouldn’t worry about this little guy.”

That just popped into my head, I must be astral projecting again. Seriously, if you don’t know what that quote is from, you fail at life, nah, just joking, look it up. Hmm, it must be late, my mind is wandering. Blogging and jamming. Jamming, is that still a socially acceptable term? Jamming to The Tangent, great stuff.

The Conclusion:

Ah, the fruits of our labor are so sweet. Remember, I know it’s hard, but remmember when I was speaking of the fluent interface, ah now you remember. Excellent, so if we take a look at our new GetItemsLikeName() method we can see the fluency at work;

we want to GetAllItems(WhereInactiveDateIsNull,(and) whereColumnValueStartsWithNameIgnoringCase).

If you can’t glean what this is doing from the naming, I am sorry, but there is no hope for you, . This fluent interface does several things, it decreases the learning curve for new-comers, increases the readability of our code, and overall increases the maintainability.

I find that the best thing to do is try to contemplate the Single Responsiblity Principle, Dont Repeat Yourself Principle, or other sundry names for it; when you are satisfying your broken tests, assuming you are testing first. I know that sometimes it can be hard but it is a fracking awesome practice to achieve. Even if you can’t see the common responsibilities at first, definitely try to find them after your tests pass. Red, Green, Refactor.