I’ve worked on a number of established codebases that implement REST APIs. In most cases I find the usual stuff you’d expect to see in such a codebase: controllers for dealing with HTTP requests, lower level services, and repositories, among other things like models and mappers. Generally the controllers make use of the services and repositories to fulfill their contracts. For example you can expect to see something like a BooksController that offers interactions with book data. The BooksController may call into objects like a BookService that offers a number of different book related operations, or a BookRepository that exposes ways to create, read, update and delete books. I’m not about to tell you there’s anything wrong with that. It’s a tried and true way to organize code and it works. However, I have noticed some less than ideal situations I keep running into when working on these codebases, which got me thinking about ways to improve. In this post I want to show you what I mean by “less than ideal situations” and offer some rather simple but weird looking techniques that can be used to handle these situations. The techniques you’ll see are really just rigorous applications of the “S” and “I” of the SOLID object oriented design principles: the “single responsibility principle” and the “interface segregation principle”, respectively.

Let’s set the stage with the interface below. This is the kind of interface you typically see on an object repository. It provides at least one method for each of the four common CRUD (Create, Read, Update, Delete) operations.

interface IBookRepository
{
  IList<Book> GetAll();
  Book Get(int bookId);
  void Update(Book book);
  void Create(Book book);
  void Delete(int bookId);
}

The interface is implemented by a class like the class below.

class BookRepository : IBookRepository
{
  public IList<Book> GetAll() { /* Details omitted */ }
  public Book Get(int bookId) { /* Details omitted */ }
  public void Update(Book book) { /* Details omitted */ }
  public void Create(Book book) { /* Details omitted */ }
  public void Delete(int bookId) { /* Details omitted */ }
}

The BooksController below obtains an instance of this interface and then uses the methods it’s interested in to achieve whatever goals it needs to without ever knowing exactly how the interface is implemented. It’s great, right?

class BooksController : ControllerBase
{
  private readonly IBookRepository _bookRepo;

  public BooksController(IBookRepository bookRepo)
  {
    _bookRepo = bookRepo;
  }

  public IList<Book> GetAllBooks()
  {
    return _bookRepo.GetAll();
  }
}

Yeah, it’s pretty great. In the snippet above you can see how a web controller may use the IBookRepository’s GetAll method to retrieve a collection of Book objects that is then returned to the HTTP client. It’s fine. It works. We love it.

What’s the problem?

Lets imagine we need to write a unit test for the GetAllBooks method of the BooksController. Aside: I wouldn’t but maybe my boss is telling me to hit specific code coverage numbers. It may look something like this.

class BooksControllerUnitTests
{
  public void GetAllBooks_ReturnsAllBooksFromRepo()
  {
    // Arrange
    var fakeBookRepo = new FakeBookRepository();
    var subject = new BooksController(fakeBookRepo);

    // Act
    var result = subject.GetAllBooks();

    // Assert
    if (result[0] != fakeBookRepo.Books[0])
      throw new Exception();
  }

  private class FakeBookRepository : IBookRepository
  {
    public List<Book> Books = new List<Book> { new Book() };
    
    public IList<Book> GetAll() { return Books; }
    public Book Get(int bookId) { return null; }
    public void Update(Book book) { /* Do nothing */ }
    public void Create(Book book) { /* Do nothing */ }
    public void Delete(int bookId) { /* Do nothing */ }
  }
}

The objective of the unit test is to assert that the controller’s GetAllBooks method returns the Book objects it got from its IBookRepository interface. Of course letting the controller call into a real implementation of IBookRepository would be impossible at worst and tricky at best so instead it hands the controller a custom implementation of the IBookRepository interface whose GetAll method returns a canned list of Book objects. It works. Mission accomplished.

Problem – We had to write unused code

Notice anything fishy about the FakeBookRepository? What’s up with those “return null” and “do nothing” implementations? A developer reading this code may be confused about why some of the interface’s methods have a “real” implementation while the others appear to be unused filler.

The IBookRepository interface has 5 methods but really the unit test is interested in controlling only the return value for the GetAll method. Could we have implemented IBookRepository another way? On the one hand you could provide real-ish implementations for all 5 interface methods. Even if you had another test that could make use of one of the 4 interface methods other than GetAll, the implementation of that interface method would probably need to be be specific to whatever scenario that other test is concerned with. It looks like the best solution is to just stub out those other 4 methods with very basic implementations like returning null or doing nothing, as I’ve shown in the example above. After all, we know they’re never going to get called so why does it matter how they’re implemented, right? What we see here is the developer fighting against the compiler. The developer knows that only the GetAll method needs an implementation but the compiler forces the developer to write useless code just to satisfy the interface contract.

We can follow the fishy smell from our unit test all the way back to the IBookRepository interface and the BooksController. By looking at just the BooksController’s constructor signature, and GetAllBooks method signature, it’s not clear what part of the IBookRepository, if any, is actually used by the GetAllBooks method. Indeed, we have to look inside the method’s implementation to discover that it calls only the interface’s GetAll method. This is knowledge that we got by reading the method’s implementation. The reason we had to implement the entire IBookRepository interface in our unit test is because we’re trying to satisfy the dependencies of a particular method through the much broader dependencies of that method’s containing class.

Writing unused code is definitely a waste of time and can lead to confusion but there’s another drawback that’s not as obvious: maintainability. Watch what happens to our unit tests when we add, change or delete methods of the IBookRepository interface. When we add a new method to the interface we immediately have compilation errors that we need to resolve by hunting down every implementation of IBookRepository and supplying yet another (in most cases useless) implementation. The same thing happens when we change an existing method signature. Perhaps worst of all is when we delete a method from the interface (when not using explicit interface implementations) because the compiler won’t complain about the method implementations that are no longer relevant. The developer may not be inclined to manually find and delete all such “dead” implementations (that nobody wanted the first place!).

Solution

It turns out that this is exactly the reason the object oriented code gods gave us the “I” in SOLID.

The interface-segregation principle (ISP) states that no client should be forced to depend on methods it does not use.

In our case the “client” is the BooksController and it’s being forced to depend on methods it doesn’t use (Get, Update, Create, Delete).

We can apply the interface segregation principle to our IBookRepository interface to make the problem go away. Let’s rewrite the interface as below. It still feels a lot like the original but each method has been moved to its own interface sub-type of the main interface which is now empty in effect. In fact the outer interface is useless here but I chose to keep it for readability reasons. You may chose to do this differently.

interface IBookRepository
{
  interface GetAll { IList<Book> GetAll(); }
  interface Get { Book Get(int bookId); }
  interface Update { void Update(Book book); }
  interface Create { void Create(Book book); }
  interface Delete { void Delete(int bookId); }
}

Let’s fix up the concrete repository implementation to match. Notice that we didn’t have to touch the method implementations at all. The class simply lists more interface implementations than it did before.

class BookRepository :
  IBookRepository.GetAll,
  IBookRepository.Get,
  IBookRepository.Update,
  IBookRepository.Create,
  IBookRepository.Delete
{
  public IList<Book> GetAll() { /* Details omitted */ }
  public Book Get(int bookId) { /* Details omitted */ }
  public void Update(Book book) { /* Details omitted */ }
  public void Create(Book book) { /* Details omitted */ }
  public void Delete(int bookId) { /* Details omitted */ }
}

Updating the controller is even more straightforward. We need only replace the IBookRepository interface dependency with the more specific IBookRepository.GetAll interface dependency.

class BooksController : ControllerBase
{
  private readonly IBookRepository.GetAll _bookRepoGetAll;

  public BooksController(IBookRepository.GetAll bookRepoGetAll)
  {
    _bookRepoGetAll = bookRepoGetAll;
  }

  public IList<Book> GetAllBooks()
  {
    return _bookRepoGetAll.GetAll();
  }
}

And finally we get to see the payoff in our unit test.

class BooksControllerUnitTests
{
  public void GetAllBooks_ReturnsAllBooksFromRepo()
  {
    // Arrange
    var fakeBookRepoGetAll = new FakeBookRepositoryGetAll();
    var subject = new BooksController(fakeBookRepoGetAll);

    // Act
    var result = subject.GetAllBooks();

    // Assert
    if (result[0] != fakeBookRepoGetAll.Books[0])
      throw new Exception();
  }

  private class FakeBookRepositoryGetAll : IBookRepository.GetAll
  {
    public List<Book> Books = new List<Book> { new Book() };
    public IList<Book> GetAll() { return Books; }
  }
}

In effect we’ve aligned our BooksController’s dependencies with those of its GetAllBooks method. That is, all dependencies of BooksController are now also dependencies of BooksController.GetAllBooks. Our unit test can now fake out exactly what BooksController.GetAllBooks needs, and nothing else. Notice also that this test is now free from unnecessary maintenance burden when we create, update and delete repository methods in the future. Awesome!

That’s not all

You may be thinking that keeping each interface down to one method is a little extreme right? Let’s take things even further. Suppose we add a new method to our BooksController to delete a book. Imagine that when a book gets deleted, the controller needs to raise some kind of an event that gets handled by some other part of the system not shown.

class BooksController : ControllerBase
{
  private readonly IBookRepository.GetAll _bookRepoGetAll;
  private readonly IBookRepository.Delete _bookRepoDelete;
  private readonly IEventManager _eventManager;

  public BooksController(
    IBookRepository.GetAll bookRepoGetAll,
    IBookRepository.Delete bookRepoDelete,
    IEventManager eventManager)
  {
    _bookRepoGetAll = bookRepoGetAll;
    _bookRepoDelete = bookRepoDelete;
    _eventManager = eventManager;
  }

  public IList<Book> GetAllBooks()
  {
    return _bookRepoGetAll.GetAll();
  }

  public void DeleteBook(int bookId)
  {
    _bookRepoDelete.Delete();
    _eventManager.RaiseBookDeleteEvent();
  }
}

We hit that compile button and… oops, our good old unit test doesn’t compile. What happened?

Problem – We broke unrelated code

Our unit test isn’t providing the BooksController with the new dependencies we added to it. But wait a minute, our test is for the GetAllBooks method which we didn’t touch at all when we added the DeleteBook method. Why should we need to update the test for GetAllBooks? That fishy smell is back and we’ll deal with shortly. For now let’s update the test to satisfy the compiler.

class BooksControllerUnitTests
{
  public void GetAllBooks_ReturnsAllBooksFromRepo()
  {
    // Arrange
    var fakeBookRepoGetAll = new FakeBookRepositoryGetAll();
    var fakeBookRepoDelete = new FakeBookRepositoryDelete();
    var fakeEventManager = new FakeEventManager();
    var subject = new BooksController(
      fakeBookRepoGetAll,
      fakeBookRepoDelete,
      fakeEventManager);

    // Act
    var result = subject.GetAllBooks();

    // Assert
    if (result[0] != fakeBookRepoGetAll.Books[0])
      throw new Exception();
  }

  private class FakeBookRepositoryGetAll : IBookRepository.GetAll
  {
    public List<Book> Books = new List<Book> { new Book() };
    public IList<Book> GetAll() { return Books; }
  }

  private class FakeBookRepositoryDelete : IBookRepository.Delete
  {
    public void Delete() { /* Do nothing */ }
  }

  private class FakeEventManager : IEventManager
  {
    public void RaiseBookDeleteEvent() { /* Do nothing */ }
  }
}

Solution

The BooksController exposes a method to get all books and another to delete a book. Do these methods really need to be part of the same class? Why did we implement these methods on the BooksController class in the first place? I suppose in a real controller there’d be some similarities in the routes mapped to these controller actions but that’s about it. Having applied the interface refactor earlier in this post, these methods don’t even depend on the same repository interface. The reality is that these methods actually have very little connection to each other. They do very different things and require very different resources to do it. To use Uncle Bob’s parlance, we could even say that the presence of these methods gives the BooksController class more than one “reason to change” which goes against the “S” in SOLID; the single responsibility principle.

The single-responsibility principle (SRP) is a computer-programming principle that states that every class in a computer program should have responsibility over a single part of that program’s functionality, which it should encapsulate.

So let’s split the GetAllBooks and DeleteBook responsibilities into their own classes.

class BooksController
{
  class GetAll : ControllerBase
  {
    private readonly IBookRepository.GetAll _bookRepoGetAll;

    GetAll(IBookRepository.GetAll bookRepoGetAll)
    {
      _bookRepoGetAll = bookRepoGetAll;
    }

    public IList<Book> Action()
    {
      return _bookRepoGetAll.GetAll();
    }
  }

  class Delete : ControllerBase
  {
    private readonly IBookRepository.Delete _bookRepoDelete;
    private readonly IEventManager _eventManager;

    Delete(
      IBookRepository.Delete bookRepoDelete,
      IEventManager eventManager)
    {
      _bookRepoDelete = bookRepoDelete;
      _eventManager = eventManager;
    }

    public void Action(int bookId)
    {
      _bookRepoDelete.Delete();
      _eventManager.RaiseBookDeleteEvent();
    }
  }
}

The action methods have been renamed to “Action” to avoid naming redundancy between the name of the method and the name of its containing class. Let’s rename our BooksControllerUnitTests class to align with the new name of the controller and clean it up.

class BooksController.GetAllUnitTests
{
  public void Action_ReturnsAllBooksFromRepo()
  {
    // Arrange
    var fakeBookRepoGetAll = new FakeBookRepositoryGetAll();
    var subject = new BooksController.GetAll(fakeBookRepoGetAll);

    // Act
    var result = subject.Action();

    // Assert
    if (result[0] != fakeBookRepoGetAll.Books[0])
      throw new Exception();
  }

  private class FakeBookRepositoryGetAll : IBookRepository.GetAll
  {
    public List<Book> Books = new List<Book> { new Book() };
    public IList<Book> GetAll() { return Books; }
  }
}

After cleaning up BooksController.GetAllUnitTests we can see it’s returned to the simple form we saw after applying the interface refactoring earlier in this post. The only differences are small changes in naming. This makes sense because of course the class holding the method under test is essentially the same as it was before the DeleteBook method was added to it. We’ve once again aligned the dependencies of the method under test with the dependencies of its containing class.

Now, if we wanted to add yet another controller action, for example to get a single book, we could add a new class following the pattern above without affecting any other part of the system at all. That means no updates or maintenance on any existing unit tests. Just add the new class with whatever it depends on and compile. Awesome!

Closing thoughts

What I’ve presented here is an admittedly aggressive take on the single responsibility principle and the interface segregation principle. It produced classes and interfaces that each contain only one method. What emerges from this is a sort of method-focused code structure where classes serve as a way to supply dependencies to a method, allowing the method signature to stay focused on the behaviour the method provides by keeping its dependencies out of the method signature itself. Think about the BooksController.GetAll class for example. Its Action method takes no parameters but it can’t be called without first constructing an instance of its containing class which requires an instance of IBookRepository.GetAll. The result is an Action method that, in a way, requires a parameter of type IBookRepository.GetAll even though there are no required parameters in the method signature itself. We can say the method signature defines what the method does and the constructor of its containing type defines what additional dependencies are needed to get the job done.

There’s one final note I want to make about what you’ve seen in this post. The breakdown of the IBookRepository interface and the BooksController worked because these types didn’t encapsulate any actual business logic or state. Instead they were really just a means to group related methods. As I’ve shown, when you get down to it, there isn’t anything technical that requires these methods to coexist on a common enclosing type. This is typical of “service” and “repository” objects but not typical of objects in general. That is to say this breakdown technique is not one that should be, or even can be, applied to all things; it’s just another tool for one’s toolbox.