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

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...