-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implementation of the task - code review #6
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
| ||
Microsoft Visual Studio Solution File, Format Version 12.00 | ||
Project("{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}") = "BackendTaskPilot", "BackendTaskPilot\BackendTaskPilot.csproj", "{85AE9448-BC75-4412-8D41-DA6C52B676D9}" | ||
EndProject | ||
Global | ||
GlobalSection(SolutionConfigurationPlatforms) = preSolution | ||
Debug|Any CPU = Debug|Any CPU | ||
Release|Any CPU = Release|Any CPU | ||
EndGlobalSection | ||
GlobalSection(ProjectConfigurationPlatforms) = postSolution | ||
{85AE9448-BC75-4412-8D41-DA6C52B676D9}.Debug|Any CPU.ActiveCfg = Debug|Any CPU | ||
{85AE9448-BC75-4412-8D41-DA6C52B676D9}.Debug|Any CPU.Build.0 = Debug|Any CPU | ||
{85AE9448-BC75-4412-8D41-DA6C52B676D9}.Release|Any CPU.ActiveCfg = Release|Any CPU | ||
{85AE9448-BC75-4412-8D41-DA6C52B676D9}.Release|Any CPU.Build.0 = Release|Any CPU | ||
EndGlobalSection | ||
EndGlobal |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
<Project Sdk="Microsoft.NET.Sdk.Web"> | ||
|
||
<PropertyGroup> | ||
<TargetFramework>net7.0</TargetFramework> | ||
<Nullable>enable</Nullable> | ||
<ImplicitUsings>enable</ImplicitUsings> | ||
</PropertyGroup> | ||
|
||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
using System.Text.Json; | ||
using System.Text.Json.Serialization; | ||
|
||
var builder = WebApplication.CreateBuilder(args); | ||
var app = builder.Build(); | ||
|
||
ExchangeRateService service = new ExchangeRateService(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// REST API to get exchange rates based on input currencies | ||
app.MapPost("/", service.GetExchangeRates); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
app.Run(); | ||
|
||
public struct CnbRatesResponse | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Line 14: Using a class instead of a struct would also be more suitable here. |
||
{ | ||
[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; } | ||
} | ||
Comment on lines
+14
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
internal class ExchangeRateService | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
{ | ||
private const string MainCurrenciesUrl = "https://api.cnb.cz/cnbapi/exrates/daily"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
public IEnumerable<Rate> GetExchangeRates(List<string> currencies) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're on the right track here, however making the method async will improve performance. |
||
{ | ||
HttpClient client = new HttpClient(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
var data = client.GetStringAsync(MainCurrenciesUrl).Result; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
var response = JsonSerializer.Deserialize<CnbRatesResponse>(data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
return response.Rates.Select(rate => rate).Where(rate => currencies.Contains(rate.CurrencyCode)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your LINQ query works as expected, however, the Select(rate => rate) is redundant and can be removed. |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
{ | ||
"iisSettings": { | ||
"windowsAuthentication": false, | ||
"anonymousAuthentication": true, | ||
"iisExpress": { | ||
"applicationUrl": "http://localhost:39873", | ||
"sslPort": 44314 | ||
} | ||
}, | ||
"profiles": { | ||
"http": { | ||
"commandName": "Project", | ||
"dotnetRunMessages": true, | ||
"launchBrowser": true, | ||
"applicationUrl": "http://localhost:5018", | ||
"environmentVariables": { | ||
"ASPNETCORE_ENVIRONMENT": "Development" | ||
} | ||
}, | ||
"https": { | ||
"commandName": "Project", | ||
"dotnetRunMessages": true, | ||
"launchBrowser": true, | ||
"applicationUrl": "https://localhost:7250;http://localhost:5018", | ||
"environmentVariables": { | ||
"ASPNETCORE_ENVIRONMENT": "Development" | ||
} | ||
}, | ||
"IIS Express": { | ||
"commandName": "IISExpress", | ||
"launchBrowser": true, | ||
"environmentVariables": { | ||
"ASPNETCORE_ENVIRONMENT": "Development" | ||
} | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
{ | ||
"Logging": { | ||
"LogLevel": { | ||
"Default": "Information", | ||
"Microsoft.AspNetCore": "Warning" | ||
} | ||
}, | ||
"AllowedHosts": "*" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With net7.0 being "end of support", it would be good practice to update to the latest patch of net8.0.