.net核心WebAPI交友应用程序的用户控制器

Question

我有这个用户控制器，它遵循存储库模式。它工作得很完美，但它很容易理解吗？这件工作质量好吗？

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");
        }

    }
}

Answer 1

误差

假设:您永远不希望返回一个500 HTTP状态代码(内部服务器错误)。它可能泄露您不想公开的信息，而且信息不够丰富。不要抛出任何异常:返回正确的HTTP状态代码，并最终使用适当的错误消息对其进行补充。例如，如果SaveAll()由于并发请求而失败，则可能返回409 (冲突)。这就引出了两点：

• 在您的代码中，您几乎不应该捕获Exception，也不会抛出它。有许多信息更丰富的异常可供使用，这些异常将很好地处理异常处理策略(您有异常处理策略吗？)
• SaveAll()返回一个布尔值只是为了抛出一个异常是，TBH，次优。如果它是一个错误条件，那么它应该是一个异常(调用者必须处理它，如果可以的话)。布尔值返回值很容易被忽略(如果没有保存数据，则不希望这种情况发生)。

您没有捕捉到任何异常，如果存储库发生了任何事情，那么API调用将因内部服务器错误而失败。

重用

在许多函数中解析当前用户ID，将其移到单独的函数中。另外，在不指定要使用的区域性(可能是Int32.Parse() )的情况下，不要使用CultureInfo.InvariantCulture。

安全

不要在存储库和控制器中使用UserParams。闻起来是因为：

• 你不能独立地改变它们。
• 您可以在查询中指定未使用的值。
• 如果添加了一个新字段，而忘了在控制器方法中处理该字段，则可能会出现漏洞。

始终验证和转换请求参数(最简单的方法是有两个独立的对象)。例如，userParams.Gender作为未经验证的字符串直接公开给您的存储库。

UpdateUser() (和LikeUser())实际上不需要用户ID作为参数。当您已经拥有服务器端时，不需要来自客户端的无用和冗余数据(必须进行验证)。

性别

除非您确切知道字符串的含义，否则不要使用==比较字符串。在这种情况下，更简单、更快的顺序比较就足够了.

不要将性别存储为字符串。如果您的应用程序仅限于二进制性别(在出生时)，那么您可以使用enum，然而.

性别比简单的二元身份要复杂得多：

• 出生时的性别不一定是二元的。
• 出生时的性别可能不是目前的性别。
• 出生时指定的性别可能不是一个人所认同的。
• 特别是在约会应用程序中，性取向也很重要(这显然是独立于性别的)。

简而言之:不要使用硬编码列表，也不要假设性别等于性取向。

性能

如果您的应用程序成功，那么您肯定希望避免搜索likes表(它的增长速度将比users表快得多)以获取条目，并且只检索返回坏请求。最好的情况是使用一个更简单的函数来确定条目是否存在，而不是读取、传输和映射条目本身。另见下一节..。

设计

我通常反对不加区分地使用仓库模式。这是一个非常宝贵的工具，但你为正确使用它而付出的复杂性代价必须是合理的。我不认为这里的大局，但你应该认真考虑是否直接使用你的ORM是足够的。据我所见，您确实需要一个域模型，而不仅仅是一个存储库。

您有一个实例字段_repo，您没有显示该代码，但我没有看到任何同步机制，我也不知道在重新创建控制器时是如何配置其资源的。通常，在控制器中，应该尽可能避免实例字段。

您的API永远不应该直接使用存储库来执行任何业务逻辑。它应该与高级模型(或ORM公开的数据层顶部的服务层)交互。例如，我会想象类似的特性如下：

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

    return Ok();
}

是的，控制器方法不应该包含更多的内容。所有的逻辑都必须是可测试的，那么在其他地方使用它就容易多了。注意，在这个虚构的例子中，我使用了一个工厂来创建服务对象:您可以直接从控制器ctor中的DI框架中获得它，但是不要在这里创建它(.new MyService())，因为您想用模拟的服务测试控制器。

注意，AddLike()可能是用存储过程实现的，这里确实需要性能。考虑到用户不应该能够为他已经喜欢的人单击“喜欢”，那么当它发生时，这肯定是一个错误条件，因此它应该触发一个异常。由于错误处理，代码可能比这更复杂，但我认为您现在明白了问题所在：

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

如果您的服务层结构良好，那么您可能可以将所有这些样板代码移动到一个函数方法中，并在所有控制器方法中重用它。