What is wrong in this c# code for simple RestApi?

Question

I received feedback:

"code test is not at the expected level there's a lack of understanding of async, HTTP clients, and just software development patterns in general. Poor structure and testing"

The simple test and solution are on GitHub https://github.com/sasa-yovanovicc/weatherapi

I'll really appreciate any help to understand what is wrong because the code works, the test works,, and covers all solutions, and honestly, I don't know what they expect.

I understand that in OOP code can be more abstracted and complex, but I can't see any purpose in making code more complex than needed to solve a given problems.

Answer 1

There are few improvements which can be made, that will not only follow modern practices, but simplify the code in some cases:

1. Use IHttpClientFactory (previously instantiating HttpClient per request was considered a bad practice - see here and here why, though now situation was improved, also see this). For possible usage patterns check the docs

2. Use strongly-typed configs for example with options pattern

3. There is literally no reason to use GetAwaiter().GetResult() in your code, for example in WeatherController:

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

Answer 2

In my view, code can be improved in this area:

1. This piece of code should be placed in Service layer. This code is placed in AstronomyController

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

   Usually, controllers should not have any logic. It should coordinate and orchestrate services.

2. If this Helper class has the StringHelper name, it will be more readable:

    public class StringHelper
   

   So it can be concluded that if StringHelper would have code snippet that will contain code which helps to work with int type, then it would be violation of Single Responsibility Principle of SOLID principles.

3. Using GetResult() can be a potential reason of a deadlock

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

   So it is better to use await:

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

4. It is better to write tests by triple A pattern. In addition, try to use abstractions when you will write code for controllers. Read more about how to test controllers here

   E.g.:

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

   Here IBrainstormSessionRepository is an abstraction. Moreover, read this article "Unit testing best practices with .NET Core and .NET Standard" is very helpful.

5. Multiple Assert should be avoided in tests. There is a very nice book by Roy Osherove’s "The Art of Unit Testing". One of the things that Osherove warns against is multiple asserts in unit tests. That is, one should generally avoid writing tests that can fail for more than one reason.

   E.g., here should be just one 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);