Although this may seem like a clear violation of SRP (single responsibility principle), it is often tempting to put simple logic in the repository — DON’T DO IT!
Here is the code that I just came across that frustrates me.
[Transaction]
public bool ShowWhatsNewLink()
{
var preference = GetAll().FirstOrDefault(p => p.Name.Equals(WhatsNewLinkVisibleKeyName));
return preference != null && Convert.ToBoolean(preference.Value);
}
It sends me on a detour cleaning this up because I have this problem of ignoring code that I know should be re-factored. Then I miss my deadline because I am busy cleaning up something that is irrelevant to my task at hand. It may seem tempting because it is so simple and obvious, but DON’T DO IT!
1. It makes the code harder to test. Since I do integration tests on my repository classes to ensure that that are bringing back the correct data, it is impossible to add unit tests that test the additional logic without going to a database. That also means that you have to modify the database as part of your test setup. GetAll() is an Nhibernate call to get our preferences. Since I want to test the functionality that this method will return false when the preference is not in the database, I have to first run an actual delete statement against the database. Like this:
var sql = "DELETE FROM Preference WHERE [Name]='{0}'".FormatWith(PreferenceRepository.WhatsNewLinkVisibleKeyName);
AdoTemplate.ExecuteScalar(CommandType.Text, sql);
var whatsNewLinkVisible = _preferenceRepository.ShowWhatsNewLink();
Assert.False(whatsNewLinkVisible, “Did not properly default to false when what’s new preference was not available.”);
Well, Dan, that wasn’t so hard. Just one extra line. Instead of mocking the data, you create sql and execute it. Ok, then consider this existing test:
[Test]
public void ShowWhatsNewLink_ShouldReturnFalse_WhenShowWhatsNewPrefenceValueIsFalse()
{
var sql = "Update Preference set Value='{0}' Where Name='{1}'".FormatWith("False",
PreferenceRepository.
WhatsNewLinkVisibleKeyName);
AdoTemplate.ExecuteScalar(CommandType.Text, sql);
var whatsNewLinkVisible = _preferenceRepository.ShowWhatsNewLink();
Assert.False(whatsNewLinkVisible, “whatsNewLinkVisible was expected to be false, but was true”);
}
Now, I have to Insert the value if it doesn’t exist or update if it does exist, otherwise, this test will pass even if the key doesn’t exist. Now I have to go through all existing tests and make modifications because the business logic is mixed with the concern of simply returning an object from the database.
2. It makes the class less useful for other purposes. Basically, if I want to get a preference, say the WhatsNewLink preference, for some other purpose, I have to write another repository method to return the what’s new link. It is already being done in the existing method, but it is turning it into a bool. So now I have two methods that have to stay in sync OR I have to change the first method to use my new method. Now my changes are cascading to parts of the system that shouldn’t be affected by just getting a preference value.
The way it should be done.
First, the repository should just return objects.
public class PreferenceRepository : HibernateRepositoryBase<Preference, int?>, IPreferenceRepository
{
// ...... //
[Transaction]
public Preference Get(int preferenceID)
{
return CurrentSession.Get(preferenceID);
}
}
Now I can restrict my test to just testing that I get the values out of the database that I put in.
Then, you create a Service class
public PreferenceService
{
Public PreferenceService(IPreferenceRepository prefRep){}// ...
}
Now when I want to test, I can insert a mock for the repository and create reliable unit tests without a connection to a database or caring what the state of the database. I just have to mock up the return value of the repository. Any tests will be independent of other tests because it does not rely on a know state of a data store.
I know it is tempting to avoid creating a new class, but it is worth it in the long run.