ASP.NET Core 2 architecture and design patterns

Question

I started a week ago to split my project into layers: business logic layer, data access layer and UI layer. I took as an example this project: https://github.com/dotnet-architecture/eShopOnWeb. I would like to know if my architecture is correct. I always tried to do thing my way and then after some weeks or more I found out that the way I was doing it wasn't the best way, so I think I will learn much more if someone with more experience could tell me what I am doing wrong.
My project link is https://github.com/qpblaze/CollegeGrades. For this, I don't want the code reviewed but the structure of the project.

Also, something is unclear to me regarding the UserService.cs. In my past projects I made my own account manager, but now I tried to use the Identity provided by Microsoft and I don't know if it's the right way to make another service which implements an interface and execute the identity methods from there. I did id like this because I thought that I might switch to another account manager and it would be much easier just implement that interface and to reduce the impact on the rest of the code. The files I want to be reviewed are:

The following code is responsible for registering, logging in the user and adding it to a role:

UserService.cs

Here is the implementation of the IUserService.cs interface. This is based on the identity managers.

public interface IUserService
{
    Task RegisterAsync(User user, string password);
    Task<string> GenerateEmailConfirmationTokenAsync(User user);

    Task SignInAsync(string email, string password);

    Task<User> FindByIdAsync(string id);
    Task ConfirmEmailAsync(string userID, string code);
    Task SignOutAsync();
}

public class UserService : IUserService
{
    #region Private Properties

    private readonly UserManager<User> _userManager;
    private readonly SignInManager<User> _signInManager;
    private readonly IRoleRepository _roleRepository;

    #endregion Private Properties

    #region Constructor

    public UserService(
        UserManager<User> userManager,
        SignInManager<User> signInManager,
        IRoleRepository roleRepository)
    {
        _userManager = userManager;
        _signInManager = signInManager;
        _roleRepository = roleRepository;
    }

    #endregion Constructor

    #region Private Methods

    private string GetProfileImageURL()
    {
        return "http://www.gravatar.com/avatar/" + Guid.NewGuid().ToString().Replace('-', '0') + "?&default=identicon&forcedefault=1&s=300";
    }

    private async Task AddToRoleAsync(User user, string name)
    {
        bool exists = await _roleRepository.RoleExistsAsync(name);
        if (!exists)
            await _roleRepository.CreateAsync(name);

        await _userManager.AddToRoleAsync(user, name);
    }

    #endregion Private Methods

    public async Task RegisterAsync(User user, string password)
    {
        // Default properties
        user.ProfileImage = GetProfileImageURL();

        var result = await _userManager.CreateAsync(user, password);
        if (!result.Succeeded)
        {
            throw new InvalidInputException(result.Errors.ToString());
        }

        await AddToRoleAsync(user, "Student");
    }

    public async Task<string> GenerateEmailConfirmationTokenAsync(User user)
    {
        return await _userManager.GenerateEmailConfirmationTokenAsync(user);
    }

    public async Task SignInAsync(string email, string password)
    {
        var result = await _signInManager.PasswordSignInAsync(email, password, false, false);

        if (!result.Succeeded)
            throw new InvalidInputException("Email", "Invalid email and/or password.");

        var user = await _userManager.FindByEmailAsync(email);
        if (!user.EmailConfirmed)
        {
            throw new InvalidInputException("Email", "The email is not cofirmed.");
        }
    }

    public async Task<User> FindByIdAsync(string id)
    {
        return await _userManager.FindByIdAsync(id);
    }

    public async Task ConfirmEmailAsync(string userID, string code)
    {
        if (userID == null)
            throw new ArgumentNullException(nameof(userID));
        if (code == null)
            throw new ArgumentNullException(nameof(code));

        var user = await FindByIdAsync(userID);

        if (user == null)
        {
            throw new ApplicationException($"Unable to load user with ID '{userID}'.");
        }

        var result = await _userManager.ConfirmEmailAsync(user, code);
    }

    public async Task SignOutAsync()
    {
        await _signInManager.SignOutAsync();
    }
}

RoleRepository.cs

This is responible for the roles management.

public interface IRoleRepository
{
    Task CreateAsync(string name);

    Task<bool> RoleExistsAsync(string name);
}

public class RoleRepository : IRoleRepository
{
    private readonly RoleManager<IdentityRole> _roleManager;

    public RoleRepository(RoleManager<IdentityRole> roleManager)
    {
        _roleManager = roleManager;
    }

    public async Task CreateAsync(string name)
    {
        var role = new IdentityRole
        {
            Name = name
        };

        await _roleManager.CreateAsync(role);
    }

    public async Task<bool> RoleExistsAsync(string name)
    {
        bool exists = await _roleManager.RoleExistsAsync(name);

        return exists;
    }
}

AccountController.cs

And this is where I use the methods from above to login and register the user.

[Authorize]
public class AccountController : Controller
{
    #region Private Properties

    private readonly IUserService _userService;
    private readonly IEmailSender _emailSender;
    private readonly IMapper _mapper;

    #endregion Private Properties

    #region Constructor

    public AccountController(
        IUserService userService,
        IEmailSender emailSender,
        IMapper mapper)
    {
        _userService = userService;
        _emailSender = emailSender;
        _mapper = mapper;
    }

    #endregion Constructor

    #region Register

    [HttpGet]
    [AllowAnonymous]
    [Route("register")]
    [RedirectLoggedUser]
    public IActionResult Register()
    {
        return View();
    }

    [HttpPost]
    [AllowAnonymous]
    [Route("register")]
    [RedirectLoggedUser]
    public async Task<IActionResult> Register(RegisterViewModel model)
    {
        if (!ModelState.IsValid)
            return View(model);

        User user = _mapper.Map<RegisterViewModel, User>(model);

        try
        {
            await _userService.RegisterAsync(user, model.Password);
        }
        catch (InvalidInputException ex)
        {
            ModelState.AddModelError(ex.Field, ex.Message);
            return View(model);
        }

        await SendConfirmationEmail(user);

        return RedirectToAction(nameof(ConfirmationEmailSent));
    }

    private async Task SendConfirmationEmail(User user)
    {
        var code = await _userService.GenerateEmailConfirmationTokenAsync(user);
        var callbackUrl = Url.EmailConfirmationLink(user.Id, code, Request.Scheme);

        await _emailSender.SendEmailConfirmationAsync(user.Email, callbackUrl);
    }

    #endregion Register

    #region LogIn

    [HttpGet]
    [AllowAnonymous]
    [Route("login")]
    [RedirectLoggedUser]
    public IActionResult LogIn()
    {
        return View();
    }

    [HttpPost]
    [AllowAnonymous]
    [Route("login")]
    [RedirectLoggedUser]
    public async Task<IActionResult> LogIn(LogInViewModel model, string returnUrl = null)
    {
        if (!ModelState.IsValid)
            return View(model);

        try
        {
            await _userService.SignInAsync(model.Email, model.Password);
        }
        catch (InvalidInputException ex)
        {
            ModelState.AddModelError(ex.Field, ex.Message);
            return View(model);
        }

        return RedirectToAction(nameof(HomeController.Index), "Home");
    }

    #endregion LogIn}

@qpBlaze Great to see users who understand how the SE system works, and react instead of leaving a complaint. — πάντα ῥεῖ
– πάντα ῥεῖ, Commented Mar 5, 2018 at 19:24
This code is well written. abstracting away the Identity framework is exactly how I did it in asp.net mvc and asp.net core. Can you include the abstraction definitions to complete the question. The assumption now is that all functions shown in the classes are from the interface. — Nkosi
– Nkosi, Commented Mar 5, 2018 at 19:33

Nkosi · Accepted Answer · 2018-03-06 12:34:13Z

This code is well written. abstracting away the Identity framework is exactly how I have done it in asp.net mvc and asp.net core. The two abstractions shown do not appear to be leaky, which is another plus.

private void AddErrors(IdentityResult result) in AccountController which was edited out could have been refactored to

private void AddErrors(InvalidInputException ex) {
    ModelState.AddModelError(ex.Field, ex.Message);
}

and used to remove repeated code

For example

[HttpPost]
[AllowAnonymous]
[Route("register")]
[RedirectLoggedUser]
public async Task<IActionResult> Register(RegisterViewModel model) {
    if (!ModelState.IsValid)
        return View(model);

    User user = _mapper.Map<RegisterViewModel, User>(model);

    try {
        await _userService.RegisterAsync(user, model.Password);
    } catch (InvalidInputException ex) {
        AddErrors(ex);
        return View(model);
    }

    await SendConfirmationEmail(user);

    return RedirectToAction(nameof(ConfirmationEmailSent));
}

Also noticed that there is no error handling in case the email confirmation fails.

However InvalidInputException is a leaky implementation concern from Identity Models. I Got around this by doing something similar to how it was done in the framework.

 /// <summary>
/// Represents the minimal result of an identity operation
/// </summary>
public interface IIdentityResult : System.Collections.Generic.IEnumerable<string> {
    bool Succeeded { get; }
}

public class DefaultIdentityResult : System.Collections.Generic.List<string>, IIdentityResult {
    private  DefaultIdentityResult(System.Collections.Generic.IEnumerable<string> errors) : base(errors) { }
    private DefaultIdentityResult(params string[] errors) : base(errors) { }
    public bool Succeeded { get { return Count == 0; } }

    public static IIdentityResult Create(params string[] errors) {
        return new DefaultIdentityResult(errors);
    }

    public static IIdentityResult Create(System.Collections.Generic.IEnumerable<string> errors) {
        return new DefaultIdentityResult(errors);
    }       

    static Lazy<IIdentityResult> success = new Lazy<IIdentityResult>(() => new DefaultIdentityResult());
    public static IIdentityResult Success {
        get {
            return success.Value;
        }
    }       

    /// <summary>
    /// Converts the value of the current <see cref="IIdentityResult"/> object to its equivalent string representation.
    /// </summary>
    /// <returns>A string representation of the current <see cref="IIdentityResult"/> object.</returns>
    /// <remarks>
    /// If the operation was successful the ToString() will return "Succeeded" otherwise it returned 
    /// "Failed : " followed by a comma delimited list of errors, if any.
    /// </remarks>
    public override string ToString() {
        return Succeeded ?
               "Succeeded" :
               string.Format("{0} : {1}", "Failed", string.Join(",", this));
    }
}

internal static class IdentityResultExtension {
    internal static IIdentityResult AsIIdentityResult(this IdentityResult result) {
        return DefaultIdentityResult.Create(result.Errors.Select(_ => _.Description));
    }
}

Using IUserService.RegisterAsync as an example refactor

public interface IUserService {
    Task<IIdentityResult> RegisterAsync(User user, string password);

    //...code removed for brevity   
}

//....  

public async Task<IIdentityResult> RegisterAsync(User user, string password) {

    // Default properties
    user.ProfileImage = GetProfileImageURL();

    var identityResult = await _userManager.CreateAsync(user, password);
    if (!identityResult.Succeeded) {
        return identityResult.AsIIdentityResult();
    }

    var result = await AddToRoleAsync(user, "Student");     

    return result;
}

private async Task<IIdentityResult> AddToRoleAsync(User user, string name) {
    bool exists = await _roleRepository.RoleExistsAsync(name);
    if (!exists)
        await _roleRepository.CreateAsync(name);

    var identityResult = await _userManager.AddToRoleAsync(user, name);

    return identityResult.AsIIdentityResult();
}

Controller action would start to look very similar to the original stock identity template

[HttpPost]
[AllowAnonymous]
[Route("register")]
[RedirectLoggedUser]
public async Task<IActionResult> Register(RegisterViewModel model) {
    if (!ModelState.IsValid)
        return View(model);

    User user = _mapper.Map<RegisterViewModel, User>(model);

    var result = await _userService.RegisterAsync(user, model.Password);
    if(result.Succeeded) {
        await SendConfirmationEmail(user); //TODO: Refactor to get result of function
        return RedirectToAction(nameof(ConfirmationEmailSent));     
    } else {
        AddErrors(result);
    }        
    return View(model);        
}

private void AddErrors(IEnumerable<string> Errors) {
    foreach (var error in Errors) {
        ModelState.AddModelError("", error);
    }
}

With the difference now being that it was not tightly coupled to Identity Framework concerns

It is a better way to throw errors or i should make a new class like ResultStatus.cs and return that instead and check in the controller if it failed? — ursachan
– ursachan, Commented Mar 5, 2018 at 19:56
Yes. That is how I've done it as well. In a way it started to look similar to Identity Framework when all was said and done. This was that in the end the code was yours to reuse and wrap any other framework — Nkosi
– Nkosi, Commented Mar 5, 2018 at 19:58
There seems to be a problem with the IdentityResultExtension. result.errors is not an IEnumerable of strings but an IEnumerable of identity errors — ursachan
– ursachan, Commented Mar 6, 2018 at 12:02
Made a slight error with the extension method. Check update. — Nkosi
– Nkosi, Commented Mar 6, 2018 at 12:35

Stack Exchange Network

ASP.NET Core 2 architecture and design patterns

1 Answer 1

You must log in to answer this question.

Hot Network Questions

ASP.NET Core 2 architecture and design patterns

1 Answer 1

You must log in to answer this question.

Related

Hot Network Questions