ASP.NET Web API for currency

Question

On the basis of the daily exchange rate data of the CB (https://ww.cbr-xml-daily.ru/daily_json.js) it is necessary to create a Web-service using ASP.Net Core, which implements 2 API methods:

1. GET /currencies - should return the list of currencies with the possibility of pagination
2. GET /currency/ - must return the exchange rate for the transferred currency identifier

I want to know what mistakes I have. What should I fix and how?

Class (and interface) who parsing website (Register as transient):

public interface ICurrentCurrency
{
 Task<IEnumerable<Valute>> GetCurrencies();
}
public class CurrentCurrency : ICurrentCurrency
{
 public async Task<IEnumerable<Valute>> GetCurrencies()
 {
 HttpClientHandler handler = new HttpClientHandler();
 IWebProxy proxy = WebRequest.GetSystemWebProxy();
 proxy.Credentials = CredentialCache.DefaultCredentials;
 handler.Proxy = proxy;
 var client = new HttpClient(handler);
 var content = await client.GetAsync("https://www.cbr-xml-daily.ru/daily_json.js");
 string json = await content.Content.ReadAsStringAsync();
 var obj = JObject.Parse(json)["Valute"].ToString();
 return JsonConvert.DeserializeObject<Dictionary<string, Valute>>(obj).Select(x => x.Value);
 }
}

In Main (.NET 6 syntax):

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddControllers();
builder.Services.AddTransient<ICurrentCurrency, CurrentCurrency>();
var app = builder.Build();
app.UseHttpsRedirection();
app.MapControllers();
app.UseRouting();
app.Run();

Model:

public class Valute
{
 [JsonProperty("ID")]
 public string Id { get; set; } 
 public string Name { get; set; }
 public string CharCode { get; set; }
 public string NumCode { get; set; }
 public int Nominal { get; set; }
 public decimal Value { get; set; }
 public decimal Previous { get; set; }
}

Class for pagination:

public class CurrencyPageParameters
{
 public const int MaxPageSize = 5;
 public int PageNumber { get; set; } = 1;
 public int _pageSize;
 public int PageSize
 {
 get => _pageSize;
 set => _pageSize = (value > MaxPageSize) ? MaxPageSize : value;
 }
}

And controller:

[ApiController]
[Route("[action]")]
public class CbrCurrencyController : ControllerBase
{
 private ICurrentCurrency _currency;
 public CbrCurrencyController(ICurrentCurrency currency)
 {
 _currency = currency;
 }
 [HttpGet]
 [ActionName("currency")]
 public async Task<Valute> GetCurrency(string id)
 {
 var item = (await _currency.GetCurrencies()).FirstOrDefault(x => x.Id == id);
 return item;
 }
 [HttpGet]
 [ActionName("currencies")]
 public async Task<IEnumerable<Valute>> Currencies([FromQuery] CurrencyPageParameters @params)
 {
 var data = (await _currency.GetCurrencies()).Skip((@params.PageNumber - 1) * @params.PageSize).Take(@params.PageSize);
 return data;
 }
}

Answer 1

GetCurrencies

• I would suggest to create a named/typed HttpClient with the proxy handler to make it reusable
  • In this SO topic you can read more about the solution

serviceCollection
 .AddHttpClient("currencyClient")
 .ConfigurePrimaryHttpMessageHandler(() => new HttpClientHandler() {
 IWebProxy proxy = WebRequest.GetSystemWebProxy();
 proxy.Credentials = CredentialCache.DefaultCredentials;
 Proxy = httpProxy 
 });

• I would also recommend to set the base url of the HttpClient rather than passing the whole url all the time

serviceCollection
 .AddHttpClient("currencyClient"), c =>
 {
 c.BaseAddress = new Uri("https://www.cbr-xml-daily.ru/");
 })
 .ConfigurePrimaryHttpMessageHandler(...);

• After these modifications your method could be simplified like this:

public async Task<IEnumerable<Valute>> GetCurrencies()
{
 var client = httpClientFactory.CreateClient(currencyClient);
 var content = await client.GetAsync("daily_json.js");
 ...
}

• I would also suggest to consider to use the ReadFromJsonAsync extension method of the HttpContent class to perform the deserialization
• It might make sense to introduce some caching here to reduce the network traffic and the overall response time

CbrCurrencyController

• You can mark your _currency private field as readonly

GetCurrency

• Inside the FirstOrDefault you might want to perform case insensitive string comparison

.FirstOrDefault(x => string.Equals(x.Id, id, StringComparison.OrdinalIgnoreCase))

• I would also suggest to return with 404 if the provided id does not have a corresponding currency
  • In this case you have to use ActionResult class

public async Task<ActionResult<Valute>> GetCurrency(string id)
{
 var currencies = await _currency.GetCurrencies();
 Valute item = currencies.FirstOrDefault(x => string.Equals(x.Id, id, StringComparison.OrdinalIgnoreCase));
 return item ?? NotFound();
}

Currencies

• I would suggest to perform some preliminary checks on your input model
  • Like the PageNumber and PageSize fields are greater than 1
• I would also recommend to check whether you are not over-paging (skipping more items than it is available)
  • Just to make it clear the Skip and the Take will not break in case of over-paging the result will be an empty collection, but it does not make too much sense to perform such request

• Aarnihauta: Thanks a lot!! What about return value in Currencies method? I returned Task<IEnumerable<Valute>>, how can I return Task<ActionResult<IEnumerable<Valute>>>?
• Peter Csala: @Aarnihauta On the happy path use return Ok(...); on the malformed/invalid parameter path use return BadRequest(...);

Answer 2

I would remove filtering/paging (.FirstOrDefault(), .Skip() etc...) logic from CbrCurrencyController actions.

This work should be done by service, not inside controller.

Another suggestion, url's, file names and sim should not be hard-coded in code, but configurable using some kind of configuration service...