Refactoring to Remove Extra Abstraction

I found this code in part of our system.

/* code altered for brevity and to isolate points of discussion. Dependency Injection, etc. removed */

public class SomeRepository : ISomeRepository {
    public SomeItem Get(int id) {
        var item = // ... Get the SomeItem from database ...
    }

    // ...
}

public static class SomeUtils {

    public static ISomeRepository Repo { get; set; }
    public static IItemsRepository ItemsRepo { get; set; }

    public static SomeItem Get(SomeItem item) {
        item.ActiveItems = ItemsRepo.GetItemsFor(item.Id);
        item.AvailableItems = ItemsRepo.GetAvailableItems(item.Id);
        item.Children = Repository.GetItemHistory(item.Id);
        return item;
    }

    // ...
}

public class SomeItemController : Controller {

    // ...

    public ISomeRepository Repo { get; set; }
    public IItemsRepository ItemsRepo { get; set; }

    public ActionResult Edit(int id) {
        var item = Repo.Get(id);
        item = SomeUtils.Get(item);
        return View("Edit", item);
    }

}

What I noticed was that every call to actually get an Item went through the static Utils class. The Utils class had become this global static thing that everything was using. The case above is simplified. Testing was a nightmare. Not only did you have inject the mocks and stubs into the repository, but you had to inject the mocks and stubs (ane more) into the static SomeUtils class.

This was, what I call, and artificial abstraction. The static utililty class had become the actual repository and the repostory wasn’t doing much of anything except single row database queries.

Changing the repository to actually get all of an object was almost trival. All that was needed was to move the current code from the static utility class Get method to the repository’s get method. As soon as this change was made, testing became eaiser and the code-base was able to move forward.

As a note, the utility class’s Get method was left in place for some legacy purposes until other code could be refactored. It was marked as [Obsolete] and will be fully replaced in the next iteration. Small steps can make a big difference.