6

I am looking at updating some of my companies existing code to allow unit tests to be implemented. To be able to do this, all repositories are being interfaced to allow DI. However, the existing code has a habit of creating 1 repository class per table in the database, meaning that one section of business logic may need to query 5 different repositories classes.

Passing in 5+ interfaces using DI seems messy and I am looking for help on how to redesign this.

This is all for an MVC 5 C# site, and an example of it is below (I have reduced the code size so that it is easier to read).

We have our controller for Account, which in this scenario manages the user sessions:

public class AccountController : Controller
{
    private AccountLogic _accountLogic { get; set; }

    public AccountController(
        ISessionRepository sessionRepository, 
        ILoginRepository loginRepository)
    {
        _accountLogic = new AccountLogic(sessionRepository, loginRepository);
    }

    public IActionResult Index()
    {
        return View();
    }

    public IActionResult Login()
    {
        return View();
    }

    public IActionResult Login(LoginModel loginModel)
    {
        if (ModelState.IsValid)
        {
            if (_accountLogic.DoesUserExist(loginModel.Username))
            {
                var existingHash= _accountLogic.GetPassword(loginModel.Username);

                if (Hash.VerifyPassword(loginModel.Password, existingHash))
                {
                    // Set Auth Cookies

                    return RedirectToAction("Index", "Home");
                }
                else
                {
                    ModelState.AddModelError(
                        "Username", 
                        "Sorry, we did not recognize " +
                            "either your username or password");
                }
            }
        }
        return View(loginModel);
    }
}

The Team tries to reduce the size of the controller by having a business class perform most of the actions. What this actually means is that the business class is very large and has many responsibilities as it has to be passed an interface for all of the repositories it has to query. In the example shown, we pass in both the Session and Login repositories, each of this is solely responsible for their corresponding tables in the database.

public class AccountLogic
{
    private ISessionRepository _sessionRepository { get; set; }
    private ILoginRepository _loginRepository { get; set; }

    public AccountLogic(
        ISessionRepository sessionRepository, 
        ILoginRepository loginRepository)
    {
        _sessionRepository = sessionRepository;
        _loginRepository = loginRepository;
    }

    public Guid CreateSession(int loginID, string userAgent, string locale)
    {
        return _sessionRepository.CreateSession(loginID, userAgent, locale);
    }

    public Session GetSession(Guid sessionKey)
    {
        return _sessionRepository.GetSession(sessionKey);
    }

    public void EndSession(Guid sessionKey)
    {
        _sessionRepository.EndSession(sessionKey);
    }

    public bool DoesUserExist(string username)
    {
        return _loginRepository.DoesUserExist(username);
    }

    public string GetPassword(string username)
    {
        return _loginRepository.GetPassword(username);
    }
}

We then have the AccountLogic class, which handles all business logic. In the example given, there is no complicated logic to be performed, so it becomes purely a chain call to the interface methods.

My question is, how do we make this easier to handle will also being testable on a larger scale? If I had a controller which needs to query 10 different repositories, surely it is not manageable to pass 15 interfaces to the controller, and then to a relevant business class? Do we need to change how repositories are handled so that there is not one class per table?

  • What does your Data Access Layer look like? Is it Entity Framework? – Robert Harvey Nov 9 '17 at 21:15
  • 2
    You do not provide enough details and your example does not look particularly difficult to handle, but would a repository class per business section make sense? – Goyo Nov 9 '17 at 21:26
  • Goyo has a point. Consider this refactoring: Introduce Parameter Object – candied_orange Nov 9 '17 at 21:41
  • 4
    It's interesting that you have everything set up for DI/IOC, yet your AccountController has a new keyword in it. The most obvious refactoring here is to reduce the number of repositories, either by combining several entities into a single repository, or by using a Generic Repository. – Robert Harvey Nov 9 '17 at 21:56
  • 2
    If your logic is querying 10 tables, it’s going to be a pain to test regardless of how many classes you use to represent them. – Telastyn Nov 9 '17 at 22:00
6

While not entirely in the style of C#, free monad API can help you here. Here's a C# example.

In a free monad API, your interface is modeled as objects:

// Static DSL for composing DbOp
public static class DB {
  public static class Session {
    public static DbOp<Guid> Create(int loginID, string userAgent, string locale) => new DbOp<Guid>.Session.Create(loginID, userAgent, locale, Return);
  }
  public static class Login {
    public static DbOp<Boolean> DoesUserExist(string username) => new DbOp<Boolean>.Login.DoesUserExist(username, Return);
  }
  public static DbOp<A> Return<A>(A value) => new DbOp<A>.Return(value);
}

// The classes that are created by the DSL. Holds the data of each operation
public abstract class DbOp<A> {
  public static class Session {
    public class Create : DbOp<A> {
      public readonly int LoginID;
      public readonly string UserAgent;
      public readonly string Locale;
      public readonly Func<Unit, DbOp<A>> Next;
      public Create(int loginID, string userAgent, string locale, Func<Unit, DbOp<A>> next) =>
        (LoginID, UserAgent, Locale, Next) = (loginID, userAgent, locale, next);
    }
  }

  public static class Login {
    public class DoesUserExist {
      public readonly string Username;
      public readonly Func<Unit, DbOp<A>> Next;
      public DoesUserExist(string username, Func<Unit, DbOp<A>> next) =>
        (Username, Next) = (username, next);
    }
  }

  public class Return : DbOp<A> {
    public readonly A Value;
    public Return(A value) => Value = value;
  }
}

This allows you to represent your entire data interface uniformly in a single file while allowing complete separation of the interface's implementation. You write programs by composing your interface objects in a free monad:

public class BusinessLogic {
  private readonly IDbInterpreter dbInterpreter;
  public BusinessLogic(IDbInterpreter dbInterpreter) {
    this.dbInterpreter = dbInterpreter;
  }

  public Guid login(username) = {
    // Compose our program of DbOp
    DbOp<Guid> program = 
      from userExists     in DB.Login.DoesUserExist(username)
      from guid           in DB.Session.Create(...)
      select guid;

    // Interpret the program to get the result
    dbInterpreter.Interpret(program);
  }
}

Interpreters look like big switch statements (pseudocode):

interface IDbInterpreter {
  A Interpret<A>(DbOp<A> ma);
}

public class DbInterpreter : IDbInterpreter {
    private readonly LoginRepository loginRepo;
    private readonly SessionRepository sessionRepo;
    DbInterpreter(LoginRepository loginRepo, 
                  SessionRepository sessionRepo) {
      this.loginRepo = loginRepo;
      this.sessionRepo = sessionRepo;
    }

    A IDbInterpreter.Interpret<A>(DbOp<A> ma) =>
      ma is DbOp<A>.Return r                ? r.Value
      : ma is DbOp<A>.Session.Create c      ? Interpret(c.Next(sessionRepo.Create(c.LoginID, c.UserAgent, c.Locale)))
      : ma is DbOp<A>.Login.DoesUserExist d ? Interpret(d.Next(loginRepo.DoesUserExist(d.Username)))
      : throw new NotSupportedException();
}

One of the benefits of using this pattern is that you can separate your DB tables separately in the system, but you can interpret them all as one DSL. That means in test you can provide a single mock interpreter to handle all your expected DB calls (and throw an exception if an unexpected call is made) (pseudocode):

class MockInterpreter : IDbInterpreter {
  A IDbInterpreter.Interpret(DbOp<A> ma) =>
    ma is DbOp<A>.Return r                ? r.Value
    : ma is DbOp<A>.Session.Create c      ? throw new Exception("") // simulate exception being thrown on session create
    // we only need to implement the operations we expect to be called in the mock interpreter
    : throw new NotSupportedException(); 
}
BusinessLogic businessLogic = new BusinessLogic(mockInterpreter);
assert businessLogic.login(username) ...

Instead of providing numerous interface mocks.

  • Um wow. Never seen lambdas in cases like this before. Not even in those links you provided. Just how pseudo is this pseudocode? – candied_orange Nov 9 '17 at 22:47
  • They aren't C# lambdas (except in the flatMap call). It's pattern matching on the DSL definition (Scalaish syntax because that is what I'm most familiar with). In my pseudocode case Db.Session.Create(...) => correlates to the ma is IO<A>.Return r ? in the C# example I linked to. – Samuel Nov 9 '17 at 23:08
  • 2
    +1 for a good answer but as somebody unfamiliar with Scala, the code is pretty difficult to read. This answer would benefit greatly from a translation to C# (or at least, C# pseudocode). – Wesley Wiser Nov 13 '17 at 19:39
  • I translated to C# using the example I provided – Samuel Nov 13 '17 at 20:25
  • This is really interesting stuff. [filed away for studying during my copious free time] – Robert Harvey Nov 13 '17 at 20:45
2

Have you tried injecting the concrete class 'AccountLogic' into 'AccountController'. Your IOC container should be able to build a concrete class without any added configuration.

public class AccountController : Controller
{
    private readonly AccountLogic _accountLogic;

    public AccountController(AccountLogic accountLogic)
    {
        _accountLogic = accountLogic;
    }
    ...
}

You can Mock a concrete class for testing by adding an empty constructor and changing the methods to 'virtual'.

public class AccountLogic
{
    private readonly ISessionRepository _sessionRepository;
    private readonly ILoginRepository _loginRepository;

    public AccountLogic() { }

    public AccountLogic(
        ISessionRepository sessionRepository, 
        ILoginRepository loginRepository)
    {
        _sessionRepository = sessionRepository;
        _loginRepository = loginRepository;
    }

    public virtual Guid CreateSession(int loginID, string userAgent, string locale)
    {
        return _sessionRepository.CreateSession(loginID, userAgent, locale);
    }
    ...
}

+1 for aligning your constructor parameters vertically instead of horizontally

Also, use

private readonly ISessionRepository _sessionRepository;

instead of

private ISessionRepository _sessionRepository { get; set; }

for injected objects.

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.