MVC 3 / C# General Design opinions

Question

I am playing with some code and was needing an opinion on a few items. I need to return a User object back to my controller from the Authentication service which is injected in the controller via Ninject. So everyone is on the same page. Here is the controller code along with some of the service code.

In the Login ActionResult, I check to see if the user exists and if they do I will authenticate them using the authentication service. Ok, easy enough it returns true|false. I also want to so some more things with the User, I already hit the database why go back and hit it again. As you can see in the authentication service I have setup a nice user object.

The big question now!!! Should I return User or IUser. My thoughts.... i don't want my controller depending on a concrete User so I was thinking wire up IUser to User via ninject and ctor Inject Iuser. Then I could set _user =_authenticationService.AuthenticateUser(userName,password);

Good, bad ugly??? thoughts???

Controller Code:

 public class UsersController : Controller
{
    private readonly IUserService _userService;
    private readonly IAuthenticationService _authenticationService;

    public UsersController(IUserService userService,IAuthenticationService authenticationService)
    {
        _userService = userService;
        _authenticationService = authenticationService;
    }

    public ActionResult Login()
    {
        return View();
    }
    [HttpPost]
    public ActionResult Login(UserLoginViewModel userLoginViewModel)
    {
        string userName = userLoginViewModel.UserName;
        string password = userLoginViewModel.Password;

        if (_userService.UserExists(userName))
        {
         //This will change
            if (_authenticationService.AuthenticateUser(userName, password))
            {

            }
        }


        return PartialView("_Login");
    }

    public ActionResult Register()
    {
        return PartialView("_Register");
    }
    [HttpPost]
    public ActionResult Register(UserRegisterViewModel userRegisterViewModel)
    {
        ModelState.AddModelError("UserName", "Testing");
        return PartialView("_Register");
    }
}

Service Code:

public class AuthenticationService:IAuthenticationService
{
    private readonly IRepository _repository;
    private readonly IEncryption _encryption;

    public AuthenticationService(IRepository repository,IEncryption encryption)
    {
        _repository = repository;
        _encryption = encryption;
    }

    // HMM! Ok I need to get the User object back to the controller, so instead of returning bool should I return User or IUser.

    public bool AuthenticateUser(string userName, string password)
    {
        try
        {
            var user = _repository.Select<Users>().Where(u => u.UserName == userName).Select(u => new User
            {
                UserId = u.UserID,
                UserTypeId = u.UserTypeID,
                UserName = u.UserName,
                Password = u.Password,
                Salt = u.Salt,
                ActivationCode = u.ActivationCode,
                InvalidLoginAttempts = u.InvalidLoginAttempts,
                IsLockedOut = u.IsLockedOut,
                LastLoginDate = u.LastLoginDate,
                Active = u.Active,
                DateCreated = u.DateCreated,
                LastUpdated = u.LastUpdated
            }).Single();

            // Check the users password hash
            if(_encryption.VerifyHashString(password,user.Password,user.Salt))
            {
                return true;
            }
        }
        catch (Exception)
        {
            return false;

        }
        // get the user from the database




        return false;
    }
}

Answer 1

I'd just lookup the User by name and check the password with bool AuthenticateUser(User user, string password). Or, if you like out parameters do it like bool AuthenticateUser(string username, string password, out User user). I think the UserExists method is useless for this scenario, since you want to to do more with the object.

Answer 2

I think this other post starts to answer your question:

Why not use an IoC container to resolve dependencies for entities/business objects?

Essentially it says that you could use a factory pattern instead of using DI.

I would also personally add that I have seen criticizm of this before in the Ninject mailing list, where it has been mentioned there are performance issues when always resolving entities via DI. Essentially there are likely so many creational references to each entity in different services and UI etc; not to mention type conversion; that your application gets a performance hit in the form of death by a million cuts (ie it just resolves the interface so often that performance becomes an issue. ). Even if it wasn't a performance issue now, im sure you want your application to remain scalable for the future.

To solve that DI performance concern you would try to direct your architecture back to a factory orientied design. This therefore implies that a factory is possibly the best candidate in the first place as opposed to useing DI to resolve entities (unless there was an adventageous reason to do so, which might not exist).

In summary:

• You probably should not have an interface of IUser in your application. Your entity shouldn't have any logic in it, so the actual type is effectively the contract you are using.

• Use a factory for Entities, no DI. This will give performance and scalability of performance.

• DI is still useful, but not recommended for entities unless there is a strong reason for it (can't think of a good reason off the top of my head).

• Usually DI would be used to promote a contract where a contract is required. A contract is implied through the User class as it should contain no logic, where as a service does contain logic and would be a great candidate for DI. The difference is, taht with the user entity, I can subclass it and create different user types, and the user class is still usefully overridden and properties etc reused, still upholding a contract like exterior. With a service, I wouldn't want a concrete dependency where I had to override all the methods on a service, as they generally aren't going to have any code sharing, and I probably wouldn't get any reuse from the existing service if I was to write a new implementation.