Implementation of the task - code review #6
Conversation
| <Project Sdk="Microsoft.NET.Sdk.Web"> | ||
|
|
||
| <PropertyGroup> | ||
| <TargetFramework>net7.0</TargetFramework> |
There was a problem hiding this comment.
With net7.0 being "end of support", it would be good practice to update to the latest patch of net8.0.
| var builder = WebApplication.CreateBuilder(args); | ||
| var app = builder.Build(); | ||
|
|
||
| ExchangeRateService service = new ExchangeRateService(); |
There was a problem hiding this comment.
Line 7: You'll want to use dependency injection rather than initialising the ExchangeRateService directly. This will help us when it comes to testing and if we ever need to swap implementations in the future.
| ExchangeRateService service = new ExchangeRateService(); | ||
|
|
||
| // REST API to get exchange rates based on input currencies | ||
| app.MapPost("/", service.GetExchangeRates); |
There was a problem hiding this comment.
Line 10: The endpoint is correctly mapped, however, for returning data you'll want to use a GET request instead of POST. Generally you'll only want to use POST for saving data.
In addition to this, although the current setup is fine for the current use, as the service grows it could get complicated. So think about moving this endpoints into its own controller class which should inherit from ControllerBase.
See more info here: https://learn.microsoft.com/en-us/aspnet/core/web-api/?view=aspnetcore-8.0
| public struct CnbRatesResponse | ||
| { | ||
| [JsonPropertyName("rates")] public List<Rate> Rates { get; set; } | ||
| } | ||
|
|
||
| public struct Rate | ||
| { | ||
| [JsonPropertyName("currencyCode")] public string CurrencyCode { get; set; } | ||
| [JsonPropertyName("rate")] public float RateValue { get; set; } | ||
| } |
There was a problem hiding this comment.
These look great, however moving these into their own files will improve the structure of the code, especially if we come to expand functionality with different data sources.
|
|
||
| app.Run(); | ||
|
|
||
| public struct CnbRatesResponse |
There was a problem hiding this comment.
Line 14: Using a class instead of a struct would also be more suitable here.
Here is some info of when to use a struct vs a class:
https://learn.microsoft.com/en-us/dotnet/standard/design-guidelines/choosing-between-class-and-struct
| [JsonPropertyName("rate")] public float RateValue { get; set; } | ||
| } | ||
|
|
||
| internal class ExchangeRateService |
There was a problem hiding this comment.
To enhance testability and adhere to SOLID principles, consider making the ExchangeRateService public and implementing an interface. This will make it easier to mock the service in unit tests and potentially swap implementations in the future.
To improve the code structure, it would be worth moving the ExchangeRateService into its file.
|
|
||
| internal class ExchangeRateService | ||
| { | ||
| private const string MainCurrenciesUrl = "https://api.cnb.cz/cnbapi/exrates/daily"; |
There was a problem hiding this comment.
It's a good idea to move the URL into the config file or environment variable. This will mean if it changes in future we can update the URL without having to recompile the entire project.
| { | ||
| private const string MainCurrenciesUrl = "https://api.cnb.cz/cnbapi/exrates/daily"; | ||
|
|
||
| public IEnumerable<Rate> GetExchangeRates(List<string> currencies) |
There was a problem hiding this comment.
You're on the right track here, however making the method async will improve performance.
|
|
||
| public IEnumerable<Rate> GetExchangeRates(List<string> currencies) | ||
| { | ||
| HttpClient client = new HttpClient(); |
There was a problem hiding this comment.
You might want to consider making HttpClient static as creating a new one for each request can have a performance impact as the service scales.
| { | ||
| HttpClient client = new HttpClient(); | ||
|
|
||
| var data = client.GetStringAsync(MainCurrenciesUrl).Result; |
There was a problem hiding this comment.
Along with the change to make the method async on line 29, consider using await rather than ".Result".
It would also be worth adding some error handling, although everything works at the moment of creation, anything could change and it would be useful to have those errors logged.
| HttpClient client = new HttpClient(); | ||
|
|
||
| var data = client.GetStringAsync(MainCurrenciesUrl).Result; | ||
| var response = JsonSerializer.Deserialize<CnbRatesResponse>(data); |
There was a problem hiding this comment.
You've got the right idea for deserializing the data, however some error handling here would also be useful. The format or makeup of the data may change in the future and as with line 33, its useful to have these errors logged.
| var data = client.GetStringAsync(MainCurrenciesUrl).Result; | ||
| var response = JsonSerializer.Deserialize<CnbRatesResponse>(data); | ||
|
|
||
| return response.Rates.Select(rate => rate).Where(rate => currencies.Contains(rate.CurrencyCode)); |
There was a problem hiding this comment.
Your LINQ query works as expected, however, the Select(rate => rate) is redundant and can be removed.
|
For new projects setting up swagger can be useful for helping diagnosing issues. Here is some more information about swagger: |
|
Along with swagger, it would be useful to setup some unit tests, here are some examples of things we could test:
|
|
Here is an overview of my comment:
Here is an example of how the folder structure could look: ExchangeRateAPI/ |
No description provided.