4
\$\begingroup\$

I've been trying to set up an n-layered application in ASP.NET MVC 5. I've converted my account controller to look like this.

I would like some feedback as to whether this is going in the right direction. What I'm trying to achieve is to off-load my controllers and leave the logic to the service. My Controller should just be doing the routing.

// all services inherit this to expose the DbContext
public class BaseService
{
    protected AppDbContext db = new AppDbContext();
}

interface IAccountService
{
    bool Login(HttpContextBase httpContext, string userName, string password);
    bool Logout(HttpContextBase httpContext, HttpSessionStateBase session);
}

public class AccountService : BaseService, IAccountService
{
    private UserManager<IdentityUser> userManager;

    public AccountService()
    {
        userManager = new UserManager<IdentityUser>(new UserStore<IdentityUser>(db));
    }

    public bool Login(HttpContextBase httpContext, string userName, string password)
    {
        var user = userManager.Find(userName, password);

        if (user != null) {
            IAuthenticationManager authenticationManager = httpContext.GetOwinContext().Authentication;

            authenticationManager.SignOut(DefaultAuthenticationTypes.ExternalCookie);
            authenticationManager.SignIn(userManager.CreateIdentity(user, DefaultAuthenticationTypes.ApplicationCookie));

            return true;
        }

        return false;
    }

    public void Logout(HttpContextBase httpContext, HttpSessionStateBase session)
    {
        IAuthenticationManager authenticationManager = httpContext.GetOwinContext().Authentication;

        authenticationManager.SignOut();
        session.Clear();
    }
}

public class AccountController : Controller
{
    private readonly IAccountService service;

    public AccountController()
    {
        service = new AccountService();
    }

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

    public ActionResult Index(IndexViewModel viewModel)
    {
        ActionResult result;

        if (service.Login(HttpContext, viewModel.UserName, viewModel.Password)) {
            result = RedirectToAction("Index", "Home");
        } else {
            result = View(viewModel);
        }

        return result;
    }

    public ActionResult Logout()
    {
        service.Logout(HttpContext, Session);

        return RedirectToAction("Index");
    }
}
\$\endgroup\$
7
\$\begingroup\$

It's looking really good. Some things I might consider that aren't necessary but could help move the code to some better abstractions and hence allow for easier unit testing of features include.

  1. Abstracting your service away from UI related actions by removing the dependency on HttpContextBase. It looks like all you are using it for is to obtain the IAuthenticationManager so I would pass that into your Login/Logout methods instead.

    interface IAccountService
    {
        bool Login(IAuthenticationManager authenticationManager, string userName, string password);
        bool Logout(IAuthenticationManager authenticationManager);
    }
    
    public bool Login(IAuthenticationManager authenticationManager, string userName, string password)
    {
        var user = userManager.Find(userName, password);
    
        if (user != null) {
    
            authenticationManager.SignOut(DefaultAuthenticationTypes.ExternalCookie);
            authenticationManager.SignIn(userManager.CreateIdentity(user, DefaultAuthenticationTypes.ApplicationCookie));
    
            return true;
        }
    
        return false;
    }
    
    public void Logout(IAuthenticationManager authenticationManager)
    {
        authenticationManager.SignOut();
        // NOTE:  Now you will have to do the session clear in your controller using this approach
            }
    
  2. If you were using a DI framework you could consider following the DIP by injecting your service into your controller. There are many DI frameworks out there (Autofac, Unity, Ninject just to name a few). This would further abstract your controller away from the service implementation details.

    // Account service supplied via Constructor injection
    public AccountController(IAccountService accountService)
    {
        service = accountService;
    }
    
  3. I noticed you have a base service. I like this approach but I think I would consider again using Dependency injection (or DI) to provide this to your service so that you can provide another level of abstraction and possibly remove your services need for this. Hence your base service might turn into something like.

    // all services inherit this to expose the DbContext
    public abstract class BaseService
    {
        protected AppDbContext db { get; private set; }
    
        protected BaseService(AppDbContext dbContext)
        {
            db = dbContext;
        }
    }
    
    public AccountService(AppDbContext dbContext) : base(dbContext)
    {
        userManager = new UserManager<IdentityUser>(new UserStore<IdentityUser>(dbContext));
    
    }
    

NOTE: If concerned that using Dependency injection means this wouldn't work, don't be. Constructor injection is a walk in the park for these frameworks.

  1. As your base service is likely to never make sense just by itself I would consider making it an abstract class to enforce implementation of the service through child classes.

  2. In the past I have liked to abstract my dbContext away behind an interface to help with further abstracting my services and provide for a final piece of the abstraction puzzle. So in your case a simple example of this would be:

    interface IDbContext
    {
       IDbSet<User> Users { get; set; }
    }
    
    class AppDbContext : IDbContext
    {
    
    }
    

By doing this you can now inject your context into your services as an interface. Hurrah!

\$\endgroup\$
  • \$\begingroup\$ These are definitely the final pieces to the puzzle, as you put it. Thanks a lot for your advice! \$\endgroup\$ – Ivo Coumans Sep 21 '14 at 12:05

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.