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.