Extension method on ICollection doesn't change the calling list?

Question

I want to create an extension method that runs on a List and accept another list:

public static void Charge<T, S>(this ICollection<T> targetList, ICollection<S> sourceList) where T : class, new()
        {
            if (targetList == null || sourceList == null)
                throw new NullReferenceException();
            targetList = new List<T>();
            foreach (var item in sourceList)
            {
                T t = new T();
                //do work on t
                targetList.Add(t);
            }

        }

however when I call it like this:

var targetList = new List<Item>();
targetList.Charge(sourceList);

the targetList doesn't change (items count = 0)

You aren't returning the target list, neither are you handling it as a reference. — Trioj, Sep 25 '17 at 19:36
If you want to make sure it's empty, why not just clear the items out of it? — HaukurHaf, Sep 25 '17 at 19:37
Why do you want someone to pass in an empty collection in the first place? Just don't have the caller provide the collection to begin with and just return a new collection. — Servy, Sep 25 '17 at 19:38
I agree with @Servy. I wouldn't even bother passing the target list as a parameter. — Trioj, Sep 25 '17 at 19:39
@MohamedAhmed so modify a list and then do nothing with it? Think about that, do you _really_ think that is what you want? — maccettura, Sep 25 '17 at 19:39
Related: [How can I get an extension method to change the original object?](https://stackoverflow.com/q/2326734) and [Impossible to use ref and out for first (“this”) parameter in Extension methods?](https://stackoverflow.com/q/2618597). — dbc, Sep 25 '17 at 19:42
Isn't this entire method just a complicated way to not call IEnumerable.Select()? — Trioj, Sep 25 '17 at 19:58

Mike Strobel · Answer 1 · 2017-09-25T20:10:56.450

The proposed method makes no sense to me.

You want to copy the contents of the source list into the target, but you first want to replace the target list to make sure it's empty? If you're going to replace the target list anyway, why not simply replace it like so?

target = source.ToList();

Also, how do you propose to implement the "do some work on t" in a generic extension method where the types of S and T are not known? Why not do the idiomatic thing, e.g.:

target = source.Select(s => Transform(s)).ToList();

Here, we assume Transform is a method capable of creating and populating a target object from a source object.

Or, you could avoid reallocating a new list by clearing the old one first:

target.Clear();
target.AddRange(source.Select(s => Transform(s)));

And if you really want to have a single invocation, you could simply wrap either of the alternatives above, e.g.:

public static List<TTarget> ToList<TSource, TTarget>(
    this IEnumerable<TSource> source,
    Func<TSource, TTarget> conversion)
{
    if (source == null)
        throw new ArgumentNullException(nameof(source));
    if (conversion == null)
        throw new ArgumentNullException(nameof(conversion));

    return source.Select(conversion).ToList();
}

Usage:

target = source.ToList(s => Transform(s));

Re: just calling ToList(): As I understand it, the problem with that is that the OP's source list is a different T than the target list. Really, IMO, I don't see why the entire code isn't something like var newThings = oldThings.Select(thing => ConvertOldThingToNewThing(thing)); — Trioj, Sep 25 '17 at 19:54
No worries. It honestly took me longer than I'd have liked to have just come to the realization that this is really just an weirdly-engineered Select() call with a transform. — Trioj, Sep 25 '17 at 20:01

score -1 · Answer 2 · answered Sep 25 '17 at 19:35

-1

You can't assign a new instance to targetList if you pass the list by value. You can add or remove or modify the content of the existing list, but if you want to assign another instance you need to add the ref keyword to allow assignments.

answered Sep 25 '17 at 19:35

Michael

1,931
2
8
22

3

You can not mark `this` argument of extension method with `ref`. – user4003407 Sep 25 '17 at 19:36
I know that. But since both are Collections you could swap arguments `public static void Charge(this ICollection ~~sourceList, ref ICollection targetList)`~~ – Michael Sep 25 '17 at 19:38

Extension method on ICollection doesn't change the calling list?

2 Answers2