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.

Ran into an issue today when I needed to get a column from a many to many relational table.  It was not in any table that mapped directly to any of my entities, so I was somewhat dumb founded on how to proceed.

 

An example would be if you have a school domain where there are multiple Students associated with multiple Courses.  You would most likely have a Student and Course entity that map to their respective database tables and a Student_Course table that does not map directly to anything.  Depending on the point of view you are working with you would probably have the Student with a collection of Course or vice versa.  For this example let’s say that we are looking at it from the perspective of a student.  As a Student I would want to see my final grade for the Course , for simplicity let’s say that the final grade is stored in the Student_Course table.  The most likely place to want that Grade is on the Course.  When you are mapping the Course you would have a property for Grade mapped out,

<class name="Course" table="Course">

    <id name="Id" column="Id" />
    <property name="Name" column="Name" />
    <property name="Description" column="Description" />
    <property name="FinalGrade" column="FinalGrade" />

</class>

but where would you specify the relational table to pull the Grade from? With the code above NHibernate will blow chunks, saying it cannot find the column FinalGrade in the Course table even though it is a property on your Course entity.

 

Well, in NHibernate there is a <join> mapping that you can use to map columns from other tables to the entity you are working with.  So, in the example we would replace the mapping:

<property name="FinalGrade"column="FinalGrade"/>

with:

<join table="Student_Course"optional="true">
    <key column="CourseId"/>
    <property name="FinalGrade"column="FinalGrade"/>
</join>

We would specify the many to many relational table with the table parameter.  Set the key column to the CourseId so NHibernate can join on the Id and you specify the property/column as normal.  Note the optional attribute, this is used to specify if you still want results returned where FinalGrade may be null.  This is more apparently useful in situations where you are loading courses and you don’t really care if there are FinalGrades associated with the Courses. 

 

Now when I am looking at a Student’s Courses the will all have the final grade populated.