Smelly Code: Direct Object Instantiation as a Testability Killer

This Smelly Code post discusses mainly a testability issue without going into further detail on other things like readability which could be improved for sure as well.

Original, smelly code

public Account ReadCompleteAccountByADUsernameAndServiceUID(string adUsername, string serviceInstanceUID)
{
    IList<Address>> addresses;
    IList<Contact> contacts;

    MasterDataBL masterDataBL = new MasterDataBL();

    Account result =  AccoDao.ReadCompleteAccountByADUsernameAndServiceUID(adUsername, serviceInstanceUID, ConnectionString.Master, out addresses, out contacts);

    result.PhoneNumber = contacts.Where(x => x.ContactType.Id == masterDataBL.GetCachedContactTypeByUid(ContactType.WellKnownUid.PHONE).Id).FirstOrDefault();
    result.MobilePhone = contacts.Where(x => x.ContactType.Id == masterDataBL.GetCachedContactTypeByUid(ContactType.WellKnownUid.MOBILE).Id).FirstOrDefault();

    result.Residence = addresses.Where(x => x.AddressTypeId == masterDataBL.GetCachedAddressTypeByUid(AddressType.WellKnownUid.RESIDENCE).Id).FirstOrDefault();
    result.Domicile = addresses.Where(x => x.AddressTypeId == masterDataBL.GetCachedAddressTypeByUid(AddressType.WellKnownUid.DOMICILE).Id).FirstOrDefault();

    return result;
}

Objections and Comments
As highlighted, the problem resides in the instantiation of the MasterDataBL class in line 6. The problem with this declaration is that it makes testing extremely difficult. When you're writing a proper unit test, then your ultimate goal is to take that unit under test out of its context and test it in complete isolation. What does this mean? Taking an object out of its context means to remove its dependencies to its neighbors. In this example the method ReadCompleteAccountByADUsernameAndServiceUID(...) is a method of my class AccountBL. Obviously that class collaborates with others, creating dependencies. Hence, they are inevitable, however, there is a distinction between the kind of coupling that is created: tight vs. loose coupling. I've written a blog post about that about half a year ago.
Instantiation is an example of tight coupling. During unit testing you won't be able to replace MasterDataBL with an appropriate stub. Consequently, if MasterDataBL is instantiated inside the method and then executes a call which ends in the database, you won't be able to prevent that.

Proposed Refactoring
A possible refactoring is therefore to convert the tight coupling into a loose coupling by introducing dependency injection or creating an appropriate factory. Usually I prefer the first approach, but the main idea is to separate the object creation from its usage.

private IMasterDataBl _MasterDataBL;
public IMasterDataBl MasterDataBL
{
    get
    {
        if (_MasterDataBL == null)
            _MasterDataBL = BlAgentConfigurationSection.GetAgent().Get<IMasterDataBl>();
        return _MasterDataBL;
    }

    set
    {
        _MasterDataBL = value;
    }
}

public Account ReadCompleteAccountByADUsernameAndServiceUID(string adUsername, string serviceInstanceUID)
{
    IList<Address> addresses;
    IList<Contact> contacts;

    Account result =  AccoDao.ReadCompleteAccountByADUsernameAndServiceUID(adUsername, serviceInstanceUID, ConnectionString.Master, out addresses, out contacts);

    result.PhoneNumber = contacts.Where(x => x.ContactType.Id == MasterDataBL.GetCachedContactTypeByUid(ContactType.WellKnownUid.PHONE).Id).FirstOrDefault();
    result.MobilePhone = contacts.Where(x => x.ContactType.Id == MasterDataBL.GetCachedContactTypeByUid(ContactType.WellKnownUid.MOBILE).Id).FirstOrDefault();

    result.Residence = addresses.Where(x => x.AddressTypeId == MasterDataBL.GetCachedAddressTypeByUid(AddressType.WellKnownUid.RESIDENCE).Id).FirstOrDefault();
    result.Domicile = addresses.Where(x => x.AddressTypeId == MasterDataBL.GetCachedAddressTypeByUid(AddressType.WellKnownUid.DOMICILE).Id).FirstOrDefault();

    return result;
}

Our codebase uses factories (as of now) which is the reason why I refactored it in that direction. Line 7 makes a call to that factory, asking for an appropriate instance of the interface IMasterDataBl (note how relying on interfaces decouples things). Moreover I've wrapped everything inside a property which allows me to explicitly set the instance of the used MasterDataBL. You guessed it, that's exactly the point where my test injects its stub while during production code the factory will be invoked.

An even - from my point of view - cleaner approach would be to rely on some kind of dependency injection framework s.t. you have a plain property for IMasterDataBl and no kind of call to a factory for retrieving the object instance, but rather the IoC container will set the right instance at the right time (Hollywood principle).

Related post:
Tackle software dependencies with IoC and Dependency Injection

This is one of a series of "smelly code" posts. Read more here or find them on Twitter.

---

Questions? Thoughts? Hit me up on Twitter