A controller on a .net core API BACKGROUND

I am working on a .Net Core API to drive a lyrics application. Users can sign up, submit artists &amp; lyrics, and earn kudos / XP points in the process. Basically a community-driven lyrics website.

CODE

I have the following ArtistsController.

[Route("api/artists")]
public class ArtistsController : Controller
{
    private readonly IArtistsService _artistsService;
    private readonly UserManager&lt;BbUser&gt; _userManager;

    public ArtistsController(IArtistsService artistsService, UserManager&lt;BbUser&gt; userManager)
    {
      _artistsService = artistsService ?? throw new ArgumentNullException(nameof(artistsService));
      _userManager = userManager;
    }

    [HttpGet]
    public async Task&lt;IActionResult&gt; GetArtists()
    {
      ArtistCardDtoCollection artistCardDtoCollection;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistCardDtoCollection = _artistsService.GetAllArtists(user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistCardDtoCollection = _artistsService.GetAllArtists("", false);
      }

      return Ok( new { artists = artistCardDtoCollection });
    }

    [HttpGet("{slug}")]
    [HttpGet("{slug}/lyrics", Name = "GetArtist")]
    public async Task&lt;IActionResult&gt; GetArtist(string slug)
    {
      if (!_artistsService.ArtistExists(slug)) return NotFound();
      ArtistOverviewDto artistOverviewDto;
      if (User.Identity.IsAuthenticated)
      {
        var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
        var user = await _userManager.FindByNameAsync(username);
        var roles = await _userManager.GetRolesAsync(user);
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, user.Id, roles.Contains("Admin"));
      }
      else
      {
        artistOverviewDto = _artistsService.GetArtistBySlug(slug, "", false);
      }

      return Ok(artistOverviewDto);
    }

    [Authorize]
    [HttpPost]
    public async Task&lt;IActionResult&gt; CreateArtist([FromBody] ArtistCreateDto artistCreateDto)
    {
      if (artistCreateDto == null) return BadRequest();
      if (!ModelState.IsValid) return BadRequest(ModelState);
      var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
      var user = await _userManager.FindByNameAsync(username);
      var artistOverviewDto = await _artistsService.AddNewArtist(user.Id, artistCreateDto);
      if (artistOverviewDto == null) return NotFound();
      return CreatedAtRoute("GetArtist", new {artistOverviewDto.Slug}, artistOverviewDto);
    }
}


I have 3 methods. One to get a list of artists, one to get a single artist and finally, one to create a new artist (you need to be logged in for that one).

How would you improve the code above? Readability of code is very important to me, and right now I don't feel it particularly reads well.
Here I will discuss only about your API, I think something may also be changed in your domain model but I don't see enough code to suggest anything really meaningful.

You have some code duplication you can avoid, extract this:

private async (string UserId, IEnumerable&lt;string&gt; Roles) GetCurrentUserAndRole()
{
    if (!User.Identity.IsAuthenticated)
        return ("", Enumerable.Empty&lt;string&gt;());

    var username = HttpContext.User.FindFirstValue(ClaimTypes.NameIdentifier);
    var user = await _userManager.FindByNameAsync(username);
    var roles = await _userManager.GetRolesAsync(user);

    return (user.id, roles);
}


Let's introduce an helper method to determine if user is an administrator:

private static bool IsAdministrator(IEnumerable&lt;string&gt; roles)
    =&gt; roles.Contains("Admin");


Now API methods might be simplified:

[HttpGet]
public async Task&lt;IActionResult&gt; GetArtists()
{
  (var userId, var roles) = await GetCurrentUserAndRole();      
  var artists = _artistsService.GetAllArtists(userId, IsAdministrator(roles));

  return Ok(new { artists = artists });
}


Exactly same thing may be applied to all the other methods. Note that names can be simplified, the list of artists does not need to be artistCardDtoCollection when simply artists is enough.

Now we have a problem. I don't know how you setup your tests but I usually like to test different things in different layers, controllers are easy to test if I can simply create an instance of an object, call a method and assert about its result. In this way I can verify all the controller logic (that should be pretty small) in simple unit testing and then check for HTTP related stuff in integration tests (slower and often tedious to write). Can I do it here? No, because GetCurrentUserAndRole() needs HttpContext and User and they're not available when I manually created an instance of ArtistsController. It's also hard to try this logic because I need the whole authentication thing to try both code paths. Let's move this logic into a separate IPermissionsService (or similar name). You get an instance of this service from your favorite DI framework and you can mock it as you prefer:

[HttpGet]
public async Task&lt;IActionResult&gt; GetArtists()
{
  var permissions = await _permissionsService.GetPermissions(HttpContext);      

  var artists = _artistsService.GetAllArtists(permissions.UserId,
      permissions.IsAdministrator);

  return Ok(new { artists = artists });
}


Now you can test it:

[TestMethod]
public async void IfUserIsAdministratorThenXyz()
{
    var artistsService = ...;
    var userManager = ...;
    var permissionsManager = new MockPermissionsManager(...);

    var controller = new ArtistsController(artistsService,
        userManager, permissionsManager);
    var response = await controller.GetArtists();

    // Check result and HTTP status code...
}


This is not the only (or the best) way to solve the problem but definitely something you should think about.

We're creating an anonymous object here, I strongly suggest to add a proper model object such as ArtistListModel. In .NET Core default naming conversion 
 for serialized JSON objects changed, now you can name properties with PascalCase and they get serialized in camelCase.

When you create a new artist and AddNewArtist() fails you're returning 404 Not Found, it's not exactly the expected HTTP status code if something went wrong.

You're not catching any exception. I do not think that this code won't ever fail in any circumstance (and a generic 500 Server Error is somehow unprofessional if it can be avoided with a more meaningful error message).

To create an object you might consider to use PUT instead of POST. I say consider because it's an opinionated guideline which I won't discuss here. Also a redirect as result of an API call isn't completely clear to me, leave this design decision to Presentation. An API call should return 201 Created and caller has the responsibility to read the object content (if interested); this is presentation logic, not domain logic. If you want to inform the client about the location of the newly created object then you can manually add Location HTTP header in your response (even if it has been originally intended to be served only with 3xx responses, it is somehow common in API interface):

201 Created
Location: http://example.com/api/v1/artists/1234


As I said I won't discuss these issues here but someone suggests to return resource content in the body of the request when returning 201 Created (in this case you do not even need to add Location header) or an empty body when returning 202 Accepted. Think about it and stick to one convention everywhere.

For documentation purposes I'd also add [ProducesResponseType] attributes to your API methods. It will help you to understand the code later and it'll make automatically generated documentation (for example with Swagger) much more complete and useful.