2
\$\begingroup\$

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;
    }
}
\$\endgroup\$

2 Answers 2

3
\$\begingroup\$

GetCurrencies

  • I would suggest to create a named/typed HttpClient with the proxy handler to make it reusable
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
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Thanks a lot!! What about return value in Currencies method? I returned Task<IEnumerable<Valute>>, how can I return Task<ActionResult<IEnumerable<Valute>>>? \$\endgroup\$
    – Aarnihauta
    Apr 28, 2022 at 12:45
  • 1
    \$\begingroup\$ @Aarnihauta On the happy path use return Ok(...); on the malformed/invalid parameter path use return BadRequest(...); \$\endgroup\$ Apr 28, 2022 at 12:55
1
\$\begingroup\$

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

\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.