这个简单的RestApi c#代码有什么问题?

Question

我收到了反馈:

"代码测试没有达到预期的水平，缺乏对异步、HTTP客户端和一般软件开发模式的理解。结构和测试不良">

简单的测试和解决方案是在GitHub https://github.com/sasa-yovanovicc/weatherapi

如果能帮助我理解哪里出了问题，我将非常感激，因为代码可以工作，测试可以工作，并且涵盖了所有的解决方案，老实说，我不知道他们想要什么。

我知道OOP代码可以更抽象、更复杂，但我看不出有什么必要把代码弄得比解决给定问题所需的更复杂。

Answer 1

可以做一些改进，这些改进不仅可以遵循现代实践，而且在某些情况下可以简化代码:

1. 使用IHttpClientFactory(以前每个请求实例化HttpClient被认为是一种不好的做法-请参阅这里和这里的原因，尽管现在情况有所改善，也请参阅此)。有关可能的使用模式，请查看docs

2. 使用强类型配置，例如选项模式

3. 实际上没有理由在代码中使用GetAwaiter().GetResult()，例如在WeatherController:

   if (response.IsSuccessStatusCode)
   {
   var weatherResponse = response.Content.ReadFromJsonAsync<WeatherResponse>().GetAwaiter().GetResult();
   

Answer 2

在我看来，代码可以在这方面得到改进:

1. 这段代码应该放在Service层。这段代码放在AstronomyController

   HttpClient httpClient = new();
   var responseAstro = await httpClient.GetAsync(requestURIAstro);
   

   通常，控制器不应该有任何逻辑。它应该协调和编排服务。

2. 如果这个Helper类有StringHelper的名字，它将更可读:

   public class StringHelper
   

   因此可以得出结论，如果StringHelper的代码片段将包含有助于使用int类型的代码，那么它就违反了SOLID原则中的单一责任原则。

3. 使用GetResult()可能是死锁的潜在原因

   var r = response.Content.ReadFromJsonAsync<ErrorResponse>()
   .GetAwaiter().GetResult();  
   

   所以最好使用await:

   var r = await response.Content
   .ReadFromJsonAsync<ErrorResponse>();
   // the other code is omitted for the brevity
   

4. 最好采用aaa模式编写测试。此外，当您为控制器编写代码时，请尝试使用抽象。阅读更多关于如何测试控制器在这里

   例如:

   public class HomeController : Controller
   {
   private readonly IBrainstormSessionRepository _sessionRepository;
   // the other code is omitted for the brevity
   

   这里IBrainstormSessionRepository是一个抽象。此外，请阅读这篇文章"。net Core和。net standard的单元测试最佳实践"。很有帮助。

5. 在测试中应避免多个Assert。Roy Osherove有一本非常好的书《单元测试的艺术》。Osherove警告不要在单元测试中使用多个断言。也就是说，通常应该避免编写可能由于多个原因而失败的测试。

   。，这里应该只有一个Assert:

   Assert.NotNull(result);
   Assert.IsType<ObjectResult>(result);
   var objectResult = result as ObjectResult;
   Assert.NotNull(objectResult);
   var model = objectResult.Value as InfoMessage;
   Assert.NotNull(model);