1

I have the following code:

const string endPoint = @"foo{0}?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 DoPagination(endPoint,response.TotalCount, itemsPerPage))
        .SelectMany(i => i.Invoices);
    response.Invoices = (response.Invoices ?? Enumerable.Empty<Invoice>()).Concat(allInvoices);
}

return response;

Here the logic is to call an API, if the total number of results exceeds pre-defined itemsPerPage, call all other pages and get the results in allInvoices and combine it with the first response and return all the results together. Since IEnumerable.Concat is used there will be a new object created and get assigned to response.

I got a code review comment specifying "All responses sent down from the wire, to the HttpClient should be immutable." The comment is particularly about response variable, which I disagree because its a locally scoped variable and does not affect the execution even if its mutable or immutable. I am not able to foresee any scenario where the code breaks because of that.

Is my argument valid?

Update:

Below is my InvoiceCollection class:

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

The response is a bad naming which leads to confusion. Here response can be renamed as fisrtCollection or something similar to that and it does not hold any information about API client.

  • Please comment why it is voted down – Anjo Mar 1 '20 at 11:26
  • 1
    Shouldn't the return type of an async method be either void or Task<...> in C#? – Doc Brown Mar 1 '20 at 11:49
  • to answer the direct question, its the type thats immutable not the variable, so scoping doesnt really come into it – Ewan Mar 1 '20 at 12:46
  • @DocBrown since the call is awaited its not wrapped in Task – Anjo Mar 1 '20 at 14:04
4

The reviewer is right:

  1. Modifying the variable in a way you do makes the code difficult to reason about. Actually, it makes the code quite cryptic.

    Imagine I'm barely using the method from your question. Something goes wrong, so I compare the logs with the actual invoices. By looking at your code, it seems at first that the only HTTP query it does is at line 3 and 4; however, sometimes the response is the actual one, and sometimes, it isn't.

    Moreover, response may contain information about the request, for instance the URI. In this case, it makes the whole thing particularly misleading: you have a response object which contains an URI to a resource, and a representation of a completely different resource. This is never OK.

  2. The class behind response doesn't belong to you: it's a different class, with a specific interface. It is unclear why the Invoice property can be changed in the first place, and chances are, the author of this class will spot this mistake and fix it later. When he does, it will break your code.

    This means that you made it more difficult for the author of the response class to refactor it. If the author doesn't have enough time to fix your code (or ask you to fix it), then there is a risk he'll just cancel the refactoring. Therefore, by mutating Invoice, you made it difficult to reduce the technical debt.

  • Thanks for the answer. For the first point, I have to make initially a call to API and whether to call the DoPagination() depends on the total count of the response . So 1st call cannot be avoided. The naming of variable is bad and hence there is a confusion. Response should have named as firstCollection or something similar because it contains only a collection of invoices along with a count and no other details like URI. – Anjo Mar 1 '20 at 13:39
  • For the second point, the logic is to return the invoices all together from the API. If there are more than a predefined results then we are suppose to call API with pageNumber till all the invoices are fetched. These results are concatenated with the initial result to form a new Invoice list. So a new list will be created only when there is multiple call happens to API – Anjo Mar 1 '20 at 13:48
  • @arseni_mourzenko My confusion is on the response part. It does not have any details regarding the client, its just set of invoices. If more invoices are there then it should be returned as a single collection. – Anjo Mar 1 '20 at 14:09
  • @Anjo: then why do you return response instead of response.Invoices? – Arseni Mourzenko Mar 1 '20 at 14:13
  • 1
    @Anjo: not sure what you mean by that. Essentially, you'll yield return each invoice as you get it inside a loop. The loop will stop when there are no invoices left, that is, when you're on the last page. If the first page is the only page, then the loop will stop there. If not, it will continue, yielding other invoices. In all cases, you'll be returning a sequence of invoices, and no object will be mutated. – Arseni Mourzenko Mar 1 '20 at 14:50
2

I think I understand where the comment is coming from. Your response object is both the response from the api and the response you are returning.

So it does seem odd to modify part of that reponse directly and then return it. Presumbaly it could make the response invaild in some way. Say you had a response.TotalNumber property you forgot to update or something.

I think the more usual approach would be to populate a return variable of a different type from the api reponse, rather than simply forwarding on the api response.

If you are doing the equivilant of wrapping the api class then, I agree, it seems overkill to make the response variable immutable, its essentially still internal to the class so you can assume that the class knows how to modify/create the return type without causing issues.

However. Techincally speaking If the object can be broken by directly changing its properties like this, making it immutable, or providing methods to change the properties in a safe fashion is superior.

Overall I think its common practice to return mutable 'data types' ie classes with no methods, from apis rather than immutable structs in c#. It's just easier and accepted even though probably not perfect.

Such simple classes just don't have breakable stuff on them and being mutable makes them easy to deserialise the incomming api data.

Otherwise you would have to write custom constructors or factories. It would be a pain.

  • Thanks for the answer. I have updated the code to show InvoiceCollection class structure. Here I have to make call API minimum 1 time to check whether the total result is more than itemsPerPage. If so then only the problem of mutation comes. Even if the Invoices get mutated I find it as ok since its the collation of all invoices. This is my whole confusion comes from. The result is no where affecting the http response – Anjo Mar 1 '20 at 14:07
  • 1
    I agree tbh. The comment is techincally correct, but practically unimportant. Ask the reviewer for examples of immutable response types els where in the code and how they handle deserialisation – Ewan Mar 1 '20 at 14:47
2

As it turns I did exactly the same code yesterday. Here it is

private async Task<IEnumerable<File>> ListRemoteFilesAsync(string category)
{
    FilesResponse response;
    int offset = -1;
    var files = new List<File>();

    do
    {
        offset++;
        response = await _client.ListFilesByAsync(category, offset*PageSize, PageSize);
        files.AddRange(response.Files);
    } while (response.Pagination.Limit * (offset + 1) < response.Pagination.Total);

    return files;
}

The difference here is that I never expose the resturn value from the api call. And I create a new collection that I return.

Note that this is a private method and the API-dependant File type will not be exposed outside of the private class scope.

  • 1
    a do while loop?!?!?!?!? burn it with fire!! – Ewan Mar 1 '20 at 17:42
  • @Ewan could you please explain the issue witb do while loop? – Anjo Mar 1 '20 at 23:29

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.