1
\$\begingroup\$

I have the following code: It works completely as expected, but I feel there is a better way to refactor it out.

The flow is as follows:

  1. Call the end point with 1 page
  2. Get the total number of results.
  3. If total count of items more than itemsPerPage.
  4. Call the API more times till results from all pages are returned, join them together as single invoice list
  5. Append with initial request and send back.

Do I need GetAllInvoices function? Reason it is kept there is because I believe the responsibility of calling each pages of API does not belong to main function.

public async Task<IEnumerable<Invoice>> GetAllInvoicesAsync()
{
    const string endPoint = @"foo/{0}/invoices?pageNum={1}&itemsPerPage={2}";
    const int itemsPerPage = xxx;
    InvoiceCollection response = await _apiClient
        .GetAsync<InvoiceCollection>(string.Format(endPoint, _apiClient.OrgId, 1, itemsPerPage));

    if (response?.TotalCount > itemsPerPage)
    {
        var allInvoices = (await GetAllInvoices(endPoint,response.TotalCount, itemsPerPage))
            .SelectMany(i => i.Invoices?? Enumerable.Empty<Invoice>());
        response.Invoices = (response.Invoices ?? Enumerable.Empty<Invoice>()).Concat(allInvoices);
    }

    return response?.Invoices; 
}

private async Task<List<InvoiceCollection>> GetAllInvoices(string endPoint,int totalCount, int itemsPerPage)
{
    var totalPages = (int)Math.Ceiling(totalCount / (double)itemsPerPage);
    var tasks = new List<Task<InvoiceCollection>>();

    for (int currentPage = 2; currentPage<=totalPages;currentPage++)
    {
        tasks.Add(_apiClient
            .GetAsync<InvoiceCollection>(string.Format(endPoint, _apiClient.OrgId, currentPage, itemsPerPage)));
    }
    var response = new List<InvoiceCollection>(await Task.WhenAll(tasks));
    return response;
}

public class InvoiceCollection
 {
     [JsonProperty("foo")]
     public IEnumerable<Invoice> Invoices { get; set; }
     public int TotalCount { get; set; }
 }

\$\endgroup\$
2
  • \$\begingroup\$ Updated the title \$\endgroup\$ – Anjo Mar 2 '20 at 14:56
  • 1
    \$\begingroup\$ I think @BCdotWEB means that the title should mention invoicing and that the current title can be asked within the body of the question. \$\endgroup\$ – pacmaninbw Mar 2 '20 at 15:30
1
\$\begingroup\$

When you have an assumption of getting a mix of return one or more values. You need to favor collection over single value return; because treating it as a collection would make things easier to deal with.

So, you can get rid of GetAllInvoices and adjust GetAllInvoicesAsync to something like this :

public async Task<IEnumerable<Invoice>> GetAllInvoicesAsync()
{
    const string endPoint = @"foo/{0}/invoices?pageNum={1}&itemsPerPage={2}";
    const int itemsPerPage = xxx;
    int pageNumber = 1; 
    int iterationCount = 0;

    var tasks = new List<Task<InvoiceCollection>>();

    var isFirstRound = true;

    do
    {   
        var response = await _apiClient.GetAsync<InvoiceCollection>(string.Format(endPoint, _apiClient.OrgId, pageNumber, itemsPerPage));

        if(isFirstRound) 
        { 
            iterationCount = response?.TotalCount <= itemsPerPage ? response?.TotalCount : (int) Math.Ceiling(totalPageItems / (double)itemsPerPage);
            isFirstRound = false;
        }

        tasks.Add(response);

        pageNumber++;
    } 
    while(pageNumber <= iterationCount)

    var invoiceResponseList = new List<InvoiceCollection>(await Task.WhenAll(tasks));

    var invoiceCollection = invoiceResponseList.SelectMany(i => i.Invoices ?? Enumerable.Empty<Invoice>());

    return invoiceCollection?.Invoices; 
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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