Safely combine two ILists, accounting for nulls

Question

I want to combine two IList's into one, either of which, or both, could be null. If both are null I want the result to be null.

I have this but I want something more elegant but still easy to read.

private IList<Filter> CombineFilters(IList<Filter> first, IList<Filter> second)
{
    IList<Filter> result = null;

    if(first != null)
        if (second != null)
            result = first.Concat(second) as IList<Filter>;
        else
            result = first;
    else if (second != null)
        result = second;

    return result;
}

Might be better asked on code review, since it appears to be working code. Might I ask why you're trying to avoid assigning `null` to `result` if that turns out to be the correct result anyway (i.e. why the `else if`)? — Damien_The_Unbeliever, Aug 15 '17 at 07:18
You'll need to `first.Concat(second).ToList()` instead of `as IList` otherwise result will be null — vc 74, Aug 15 '17 at 07:19
@Damien_The_Unbeliever the `else if` happens when `first` is `null` and `second` isn't — Bobbler, Aug 15 '17 at 07:49
@Bobbler - but consider the two cases - either `second` is null or it is not. If `second` is not null, you want the result to be equal to `second`. If `second` is null, you want the result to be null. In either case, no conditional logic is required, and you can just *always* assign `result = second` if `first` turns out to be null. — Damien_The_Unbeliever, Aug 15 '17 at 07:53
Improper usage of the `as` operator. If you would have used a normal cast instead (as you should have), you would have realized that this throws an `InvalidCastException` because the result of a `Concat` is not an `IList`, but a somewhat "smart" iterator type (which also happens to be a read-only interface to the underlying data). You should use normal casts instead of `as` by default, and `as` only if you know what you're doing. — dialer, Aug 15 '17 at 08:00
@Damien_The_Unbeliever No. Second doesn't take precedence. If both are null I want to return null, or at least an empty List, as others have suggested. If both are non-null I want a concatenation of both. — Bobbler, Aug 15 '17 at 09:27
I'll try *one more time*. To match your spec, you could simply say "if first is null, I'll return second. If second is null, I'll return first. Only if both are non-null do I need to do anything special." If, in that first case, first was null and you returned second, it *doesn't matter* what value it held, it still matches your spec. — Damien_The_Unbeliever, Aug 15 '17 at 09:39
@Damien_The_Unbeliever Ha! I got it. Thanks for your patience friend. — Bobbler, Aug 15 '17 at 09:45

score 4 · Accepted Answer · answered Aug 15 '17 at 07:22

4

I would think it’s a bit of a weird behavior if I call your CombineFilters with two lists (one possibly null) and get back a list which is not a copy. That would result in the following behavior:

List<Filter> filters1 = new List<Filter>() { new Filter() };
List<Filter> filters2 = null;

Console.WriteLine(filters1.Count); // 1

var filters = CombineFilters(filters1, filters2);
filters.Add(new Filter());

Console.WriteLine(filters.Count); // 2
Console.WriteLine(filters1.Count); // also 2

I’m not sure if I would really expect this behavior.

So I would suggest you to always ensure that there will be a new list in the end. This allows you to make this really short:

private IList<Filter> CombineFilters(IList<Filter> first, IList<Filter> second)
{
    return (first ?? Array.Empty<Filter>()).Concat(second ?? Array.Empty<Filter>()).ToList();
}

answered Aug 15 '17 at 07:22

poke

369,085
72
557
602

Thanks for the elegant answer. Why is having a new list better? I prefer to only instantiate if necessary. – Bobbler Aug 15 '17 at 07:51
1

It’s not necessarily better but it results in a more consistent experience. Since the method returns an `IList` I would expect to be allowed to modify it; but modifications should probably not end up affecting my original lists. You could avoid this if you would return an `IEnumerable` though, since that is not mutable by design. – poke Aug 15 '17 at 07:56
Amazing answer. – pim Aug 15 '18 at 16:17

Gilad Green · Answer 2 · 2017-08-15T07:25:51.067

Use Enumerable.Concate together with the ?? operator:

if(first == null && second == null)
    return null;
return Enumerable.Concat(first ?? Enumerable.Empty<IFilter>(), 
                         second ?? Enumerable.Empty<IFilter>()).ToList();

If either first or second are null a new list will be used instead for the concat.

I think that in case both lists are null then still an initialize collection should be returned. In that case just remove the if statement. Also maybe return an IEnumerable instead of IList:

private IEnumerable<Filter> CombineFilters(IEnumerable<Filter> first, IEnumerable<Filter> second)
{
    return Enumerable.Concat(first ?? Enumerable.Empty<IFilter>(), 
                             second ?? Enumerable.Empty<IFilter>());
}

Returning the IEnumerable instead of the IList depends on your scenario. Read more here:

Puppy · Answer 3 · 2017-08-15T07:21:04.803

1

private IList<Filter> CombineFilters(IList<Filter> first, IList<Filter> second)
{
    if (first == null && second == null) return null;
    first = first ?? new List<Filter>();
    second = second ?? new List<Filter>();
    return first.Concat(second).ToList();
}

edited Aug 15 '17 at 07:21

answered Aug 15 '17 at 07:20

Puppy

144,682
38
256
465

How about `Enumerable.Empty()` instead? – haim770 Aug 15 '17 at 07:21
I missed that bit. Seems a bit odd to have a function where the contract is null-is-empty on the input but may return null for the output. – Puppy Aug 15 '17 at 07:21

score 1 · Answer 4 · edited Aug 15 '17 at 12:23

You could do something like this:

private IList<Filter> CombineFilters(IList<Filter> first, IList<Filter> second)
{

    if (first == null && second == null) 
    {
        return null;
    }

    return (first ?? Enumerable.Empty<Filter>())
            .Concat(second ?? Enumerable.Empty<Filter>())
            .ToList();
}

That said, it's a good practice to avoid null collections entirely; if there are no elements, you just return an empty collection. This way, you don't need to check for null all the time. This may not be possible to do in your case, but if it is, it will make your code quite a bit cleaner.

Also, consider using IEnumerable<T> rather then IList<T> as it is a more general abstraction (and also because IList<T> is a broken, leaky abstraction - implementations throw exceptions (!) for methods they don't support). On the other hand, I can see that this is a private method, and if you are actually always passing List<Filter>, it's probably better to declare the parameters as List<Filter>.

EDIT:
Just noticed my answer is very similar to the one posted by Gilad Green, which I didn't see before.

@Bobbler: On it's own merits, `IList` is OK, however, in earlier versions of the .NET framework it was treated (more or less) as an abstraction of the notion of a 'collection'. So, many collections in .NET 2.0 implement IList, even though many of them don't support a significant subset of the methods defined by the interface. Their solution was to throw `NotSupportedException`-s, which is pretty horrible. This violates Liskov subtitution principle, and reduces the usefulness of `IList`. Then they figured that collections are really things you can enumerate (`IEnumerable`). — Filip Milovanović, Aug 15 '17 at 12:31

Safely combine two ILists, accounting for nulls

4 Answers4