Should we consider immutability for local scoped variables

Question

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.

Answer 1

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.

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

Answer 3

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.