4
\$\begingroup\$

I have this user controller that follows the repository pattern. It works perfect, but is it easy to understand? Is this good quality work?

using System;
using System.Collections.Generic;
using System.Security.Claims;
using System.Threading.Tasks;
using AutoMapper;
using DatingApp_API.Data;
using DatingApp_API.DataTransferObjects;
using DatingApp_API.Helpers;
using DatingApp_API.Models;
using Microsoft.AspNetCore.Authorization;
using Microsoft.AspNetCore.Mvc;

namespace DatingApp_API.Controllers
{
    [ServiceFilter(typeof(LogUserActivity))]
    [Authorize]
    [Route("api/[controller]")]
    [ApiController]
    public class UsersController : ControllerBase
    {
        private readonly IDatingRepository _repo;
        private readonly IMapper _mapper;
        public UsersController(IDatingRepository repo, IMapper mapper)
        {
            _mapper = mapper;
            _repo = repo;
        }

        [HttpGet]
        public async Task<IActionResult> GetUsers([FromQuery]UserParams userParams)
        {
            var currentUserId = int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value);

            var userFromRepo = await _repo.GetUser(currentUserId);

            userParams.UserId = currentUserId;

            if(string.IsNullOrEmpty(userParams.Gender))
            {
                userParams.Gender = userFromRepo.Gender == "male" ? "female" : "male";
            }

            var users = await _repo.GetUsers(userParams);
            var usersToReturn = _mapper.Map<IEnumerable<UserForListDTO>>(users);

            Response.AddPagination(users.CurrentPage, users.PageSize, users.TotalCount, users.TotalPages);
            return Ok(usersToReturn);
        }

        [HttpGet("{id}", Name = "GetUser")]
        public async Task<IActionResult> GetUser(int id)
        {
            var user = await _repo.GetUser(id);
            var userToReturn = _mapper.Map<UserForDetailedDTO>(user);

            return Ok(userToReturn);
        }

        [HttpPut("{id}")]
        public async Task<IActionResult> UpdateUser(int id, UserForUpdateDTO userForUpdateDTO)
        {
            if(id != int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value))
            return Unauthorized();

            var userFromRepo = await _repo.GetUser(id);

            _mapper.Map(userForUpdateDTO, userFromRepo);

            if(await _repo.SaveAll())
                return NoContent();

            throw new Exception($"Updating user {id} failed on save");
        }

        [HttpPost("{id}/like/{recipientId}")]
        public async Task<IActionResult> LikeUser(int id, int recipientId)
        {
            if(id != int.Parse(User.FindFirst(ClaimTypes.NameIdentifier).Value))
                return Unauthorized();

            var like = await _repo.GetLike(id, recipientId);

            if(like != null)
                return BadRequest("You already like this user");

            if(await _repo.GetUser(recipientId) == null)
                return NotFound();

            like = new Like
            {
                LikerId = id,
                LikeeId = recipientId
            };

            _repo.Add<Like>(like);

            if(await _repo.SaveAll())
                return Ok();

            return BadRequest("Failed to like user");
        }

    }
}
\$\endgroup\$
6
\$\begingroup\$

Errors

An assumption: you never ever want to return a 500 HTTP status code (Internal server error). It may leak information you do not want to expose and it's not informative enough. Do not throw any exception: return the correct HTTP status code and eventually supplement it with the an appropriate error message. For example if SaveAll() failed because a concurrent request you may return 409 (Conflict). This leads to two more points:

  • In your code you should almost never catch Exception and never throw it. There are many, more informative, exceptions to use which will play well with your exception handling policy (do you have one, right?).
  • SaveAll() returning a boolean just to throw an exception is, TBH, sub-optimal. If it's an error condition then it should be an exception (and caller MUST handle it, if it can). A boolean return value can too easily be ignored (and you don't want this to happen if data has not been saved).

You're not catching any exception, if anything happens with the repository then API calls will fail with an internal server error.

Reuse

You parse the current user ID in many functions, move it out to a separate function. Also do not use Int32.Parse() without specifying the culture you want to use (probably CultureInfo.InvariantCulture).

Security

Do not use UserParams both in your repository and in your controller. It smells because:

  • You can't change them independently.
  • You have unused values you can specify in the query.
  • If you add a new field and you forget to handle it in a controller method you might end up with a vulnerability.

ALWAYS validate and translate request parameters (and the easiest way is to have two separate objects). For example userParams.Gender is directly exposed to your repository as an unvalidated string.

UpdateUser() (and LikeUser()) does not actually need the user ID as parameter. Do not require useless and redundant data from the client (which must be validated) when you already have it server-side.

Gender

Do not compare strings using == unless you exactly know what it means. In this case an even simpler and faster ordinal comparison is enough however...

Do not store the gender as a string. If your app is limited to binary genders (at birth) then you may use an enum, however...

Gender is much more complex than a simple binary identity:

  • Gender at birth is not necessarily binary.
  • Gender at birth may not be the current gender.
  • The assigned gender at birth may not be what a person identifies with.
  • Especially in a dating app it's also important the sexual orientation (which is obviously independent from gender).

In short: do not use an hard-coded list and do not assume gender equals sexual orientation.

Performance

If your app will be successful then you definitely want to avoid to search the likes table (which will grow much faster than the users table) for an entry and to retrieve it only to return Bad request. Best case scenario is to have a simpler function to determine if the entry exists instead of reading, transmitting and mapping the entry itself. See also the next section...

Design

I'm generally against the indiscriminate use of the Repository pattern. It's an invaluable tool but the price you pay in complexity to use it (properly) must be justified. I do not see the big picture here but you should seriously consider if directly using your ORM is enough. From what I see you really need a domain model, much more than a repository.

You have an instance field _repo, you do not show that code but I do not see any synchronization mechanism and I do not see how you dispose its resources when controller is re-created. Generally, in your controllers, you should avoid instance fields as much as possible.

Your API should never ever work directly with the repository to perform any business logic. It should interact with an high-level model (or a Service layer on the top of the Data layer exposed by your ORM). For example I'd imagine the Like feature like this:

public async Task<IActionResult> LikeUser(int recipientId)
{
    using (var service = _serviceFactory.Create())
        await service.AddLike(GetCurrentUserId(), recipientId);

    return Ok();
}

Yes, controller methods should not contain much more than this. All the logic must be TESTABLE then it is much easier to have it elsewhere. Note that in this fictional example I used a factory to create the service object: you may get it directly from your DI framework in the controller ctor but do not create it here (...new MyService()) because you want to test the controller with a mocked service.

Note that the AddLike() is probably implemented with a stored procedure, you really need performance here. Given that user should not be able to click Like for someone he already liked then when it happens it's definitely an error condition and as such it should trigger an exception. Code is probably more complex than this because of error handling but I think you now got the point:

try
{
    ...
}
catch (ItemNotFoundException e)
{
    return NotFound(...);
}
catch (ConcurrencyException e)
{
    // TODO: try again?
}

If you structure well your service layer then you can probably move all this boilerplate code into a function method and reuse it in all controller methods.

\$\endgroup\$
  • \$\begingroup\$ Given that user should not be able to click Like for someone he already liked then when it happens it's definitely an error condition and as such it should trigger an exception. You should usually safely ignore if the user already done some action and return successful response. This is not an exception, it happens often under normal circumstances. User opened multiple tabs, windows, browsers, devices,... or he has spotty connection, gets on an elevator, drives throuh a tunnel. If you really need to make it known to the client you can return 201 the first time and 200 subsequently. \$\endgroup\$ – abuzittin gillifirca Mar 14 at 13:36
  • \$\begingroup\$ Maybe this more an UI problem than an API problem but I'd agree with you, that's the behavior user expects and the 201/200 pair is a nice solution to inform the client. \$\endgroup\$ – Adriano Repetti Mar 14 at 13:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.