Decoupling of validation and data access in ASP.NET

Question

I've been asigned to a project no dev has touched in a long time. It's an ASP.NET MVC 4 application.
It appeared to be well coded, but had no testing and the database migration folder is not present.
For further development I'm given time for minimal testing to ensure that my changes won't break the code.
When writing a Unit Test for a controller I noticed, that the code is not testable, because not only are results returned, but there is a lot of DB magic going on.
Some sample:

[HandleError]
public class AccountController : Controller
{
    private readonly ILog _logger = LogManager.GetLogger(MethodBase.GetCurrentMethod().DeclaringType);

    public IFormsAuthenticationService FormsService { get; set; }
    public IMembershipService MembershipService { get; set; }

    protected override void Initialize(RequestContext requestContext)
    {
        // ... Init code
    }

    [HttpPost]
    public ActionResult LogOn(LogOnModel model, string returnUrl)
    {
        {
            if (ModelState.IsValid)
            {
                var dbEntities = new PortalEntity();
                _logger.Debug("User " + model.UserName + " is trying to login");

                var userId = (from us in dbEntities.aspnet_Users
                              where us.UserName.Equals(model.UserName)
                              select us.UserId).FirstOrDefault<Guid>();

                var userprojects = (from up in dbEntities.T_USER_PROJECT
                                    where up.UPR_USER_ID.Equals(userId)
                                    select up);

                //NEU alle Benutzer können sich anmelden
                var projectId = (from u in dbEntities.T_PROJECT
                                 join v in userprojects on u.PRO_ID equals v.UPR_PROJECT_ID
                                 where u.PRO_IS_ACTIVE
                                 select u.PRO_ID).FirstOrDefault();
                var loginAllowed = dbEntities.T_PROJECT.Any(x => x.PRO_ID.Equals(projectId) && x.PRO_STARTED);
                //loginAdmin = ;

                var userRole = dbEntities.aspnet_Roles.Single(x => x.RoleName.Equals("Administrators")).RoleId;
                var isUserAdmin = dbEntities.aspnet_Users.Where(x => x.aspnet_Roles.Any(y => y.RoleId.Equals(userRole))).Any(x => x.UserId.Equals(userId));

                if (MembershipService.ValidateUser(model.UserName, model.Password) && (loginAllowed || isUserAdmin))
                {
                    _logger.Debug("User " + model.UserName + " existiert und möchte sich nun einloggen");
                    FormsService.SignIn(model.UserName, model.RememberMe);
                    if (isUserAdmin)
                        return RedirectToAction("Index", "Admin");

                    return RedirectToAction("Edit", "Stage1");
                }
                var errMsg = ViewResources.ErrorUserOrPasswordWrong;
                if (!loginAllowed && !isUserAdmin)
                {
                    errMsg = ViewResources.ErrorNoEntryAllowed;
                }
                ModelState.AddModelError(string.Empty, errMsg);
            }

            // If we got this far, something failed, redisplay form
            return View(model);
        }
    }
}

I abstracted a class AccountService that handels all db access:

private class AccountService {
        private PortalEntity dbEntities = new PortalEntity();

        public Guid GetUserId(LogOnModel model) {
            Guid userId = (from us in dbEntities.aspnet_Users
                           where us.UserName.Equals(model.UserName)
                           select us.UserId).FirstOrDefault<Guid>();
            return userId;
        }

        public IQueryable<T_USER_PROJECT> GetProjectsForUserId(Guid userId) {
            IQueryable<T_USER_PROJECT> userprojects = (from up in dbEntities.T_USER_PROJECT
                                                       where up.UPR_USER_ID.Equals(userId)
                                                       select up);
            return userprojects;
        }

        public int GetProjectId(IQueryable<T_USER_PROJECT> userprojects) {
            int projectId = (from u in dbEntities.T_PROJECT
                             join v in userprojects on u.PRO_ID equals v.UPR_PROJECT_ID
                             where u.PRO_IS_ACTIVE
                             select u.PRO_ID).FirstOrDefault();
            return projectId;
        }

        public bool IsLoginAllowed(int projectId) {
            bool loginAllowed = dbEntities.T_PROJECT.Any(x => x.PRO_ID.Equals(projectId) && x.PRO_STARTED);
            return loginAllowed;
        }

        public Guid GetUserRoleGuid() {
            Guid userRole = dbEntities.aspnet_Roles.Single(x => x.RoleName.Equals("Administrators")).RoleId;
            return userRole;
        }

        public bool IsUserAdmin(Guid userRole, Guid userId) {
            bool isUserAdmin = dbEntities.aspnet_Users.Where(x => x.aspnet_Roles.Any(y => y.RoleId.Equals(userRole))).Any(x => x.UserId.Equals(userId));
            return isUserAdmin;
        }
    }

Resulting in the new LogOn method:

[HttpPost]
    public ActionResult LogOn(LogOnModel model, string returnUrl)
    {
        AccountService service = new AccountService();
        {
            if (ModelState.IsValid)
            {
                _logger.Debug("User " + model.UserName + " is trying to login");

                var userId = service.GetUserId(model);

                var userprojects = service.GetProjectsForUserId(userId);

                var projectId = service.GetProjectId(userprojects);
                var loginAllowed = service.IsLoginAllowed(projectId);

                var userRole = service.GetUserRoleGuid();
                var isUserAdmin = service.IsUserAdmin(userRole, userId);

                if (MembershipService.ValidateUser(model.UserName, model.Password) && (loginAllowed || isUserAdmin))
                {
                    _logger.Debug("User " + model.UserName + " existiert und möchte sich nun einloggen");
                    FormsService.SignIn(model.UserName, model.RememberMe);
                    if (isUserAdmin)
                        return RedirectToAction("Index", "Admin");

                    return RedirectToAction("Edit", "Stage1");
                }
                var errMsg = ViewResources.ErrorUserOrPasswordWrong;
                if (!loginAllowed && !isUserAdmin)
                {
                    errMsg = ViewResources.ErrorNoEntryAllowed;
                }
                ModelState.AddModelError(string.Empty, errMsg);
            }

            // If we got this far, something failed, redisplay form
            return View(model);
        }
    }

Issues I'm having:

• Languages english and german are mixed in both comments and logs (prefer eng > ger?)
• Magic strings (x.RoleName.Equals("Administrators")), what to do with those?
• I made AccountService a private class, should I define a base class or interface Service?
• Should I be putting all these services into a dir /services?
• Is this approach even viable / necessary?
• Do I have to DI a service in every method or one per controller?

Any feedback is appriciated.

Answer 1

I guess you'll find a lot of redundancy in the code since its been held by the controllers.

Your idea of making a service is a good idea, you may need to add different services, but at the end you'll have services that can be reused, avoiding code redundancy. This would help ease things more and more. So, continue in this approach. Interface or abstract, that depends on your implementation. Go with what you see best fits your needs. You can also define both if needed. And yes, defining them under a new namespace, would be better.

Do you need to do all that ? it depends. If the application will be continued and there is no new migrations. Then, I don't think it needs that, you might make changes whenever needed (like do one controller on each time you're requested to make changes on any of them). little by little, you'll have a full updated project.

However, if there is another migration process, then you need to reconsider your choices, see what needs your efforts and what's not.

For the magic strings, use Enum.

I forgot the first issue, for which language you should favor over the other, mostly English is your choice, but from a back experience, If there is some work guidelines for developers, then it should be mentioned in that guidelines, if not, then see what is the official communication language in your work (emails ..etc) and enforce it in the code as well. You can do your comments in both languages (each comment would hold English and translation to German). I usually use English even if the application comments are written in a different language, because :

1. English is an official language in most companies in my country.
2. I'm coding in English, why should I write comments in different language ? it would be an awkward code style ;).
3. It's a good way to practice your language and keep it rolling, specially if you're in a place where it's rarely used.