atc-net/atc-coding-rules

Rule: MA0016 Prefer return collection abstraction instead of implementation

davidkallesen opened this issue · 7 comments

davidkallesen commented

Rule summary

Value
Title MA0016 Prefer return collection abstraction instead of implementation
CheckId MA0016
Category ?
Link https://github.com/meziantou/Meziantou.Analyzer/blob/master/docs/Rules/MA0016.md

What is the problem

List<T> or IList<T>

In a theoretical world, it sounds good to always use interfaces over concrete implementation. But this rule does not seem very good in practice and with reference to this post https://stackoverflow.com/questions/400135/listt-or-ilistt
with answers to the same questions by Arec Barrwin, I will suggest the general rule should be just use List<T> and if you want to over-engineering then feel free to use IList<T>.

Suggestion

disable this rule in root .editorconfig with dotnet_diagnostic.MS0016.severity = none

perkops commented

When returning from a extension method - e.g.:

        public static IEnumerable<Api.Generated.Contracts.Machines.Order> ToOrders(this IList<Entities.Order> orders)
        {
            if (orders == null)
            {
                throw new ArgumentNullException(nameof(orders));
            }

            return orders
                .Select(x => x.ToOrder())
                .ToList();
        }

And then utilizing this extension method from a handler - we get the following code:

        private async Task<GetOrdersByQueryResult> InvokeExecuteAsync(GetOrdersByQueryParameters parameters, CancellationToken cancellationToken)
        {
            var orderQueryOptions = parameters.ToOrderQueryOptions();
            var entities = await repository.GetOrdersByQueryAsync(orderQueryOptions, cancellationToken);
            return GetOrdersByQueryResult.Ok(entities.ToOrders().ToList());
        }

Do we really want this entities.ToOrders().ToList() ?

egil commented

As a general rule of thumb, I try to make my methods take as basic an abstraction as I can in input arguments, e.g. accepting IEnumerable<string> instead of List<string>, as that leaves the caller with the most options.

For output/return types, I like to avoid concrete implementations where possible, especially in public APIs in libraries, but do make them as feature rich as I can safely do (to keep changeability high in the future without breaking users). If I e.g. know that my collection will always be in memory, and have a known count, then IReadOnlyCollection<string> or IReadOnlyList<string> are good options, if the user has no reason to change the result. Otherwise ICollection<string> or IList<string> works well, but still allow me to add business logic later if needed, by creating a custom implementation of those interfaces, which returning a List<string> wont.

An added benefit of using the abstract collection types in return values is that you can more easily use Array.Empty<T>() if you want to return an default empty collection.

UPDATE: To make this a bit more explicit, I would write the two methods @perkops has above like this:

public static IEnumerable<Api.Generated.Contracts.Machines.Order> ToOrders(this IEnumerable<Entities.Order> orders)
{
    if (orders is null)
    {
        throw new ArgumentNullException(nameof(orders));
    }

    return orders.Select(x => x.ToOrder());
}

In the ToOrders method, we do not need more than an IEnumerable as input to do the work, and the return type in this case should NOT be more specific, since the user might chain other LINQ methods on the result from here. If the return type was e.g. IList<T> we would have to call ToList() in the method. Doing so means that users cannot do data.ToOrders().OrderBy(x => ...) without causing unnecessary allocations.

The InvokeExecuteAsync isn't the problem here actually. It is the GetOrdersByQueryResult.Ok that is a problem, because it takes an List<T> as input. That likely only need to be an IEnumerable<T> as well, which leaves the caller with the widest possible options when calling the method -- a good thing, e.g.:

public static GetOrdersByQueryResult Ok(IEnumerable<T> data)
{
    // ...
}

That allows us to call with a list, a collection, an array, or an unevaluated LINQ enumerable:

private async Task<GetOrdersByQueryResult> InvokeExecuteAsync(GetOrdersByQueryParameters parameters, CancellationToken cancellationToken)
{
    var orderQueryOptions = parameters.ToOrderQueryOptions();
    var entities = await repository.GetOrdersByQueryAsync(orderQueryOptions, cancellationToken);
    return GetOrdersByQueryResult.Ok(entities.ToOrders());
}

It is up to the user of OK method to decide if they want to call e.g. ToArray() or ToList() on their data before passing the data to the OK method. If the OK method doesn't need to iterate over the enumerable more than once, then IEnumerable is safe. If it needs to do multiple iterations over the data, then an IReadOnlyCollection is a good alternative, then IReadOnlyList if an indexer is needed, and then ICollection or IList following that.

In general, I find that input collections to public methods is not mutated in the receiving method, they are usually only read, so most of the time, the read only types works very well, and leaves the caller with many options.

egil commented

For test projects, this should be none though.

MrAndersPedersen commented

Closed on normal projects but set to NONE on test projects.