C# better to return a List or modify an existing one?

Question

Is it, in general, better to do this:

public void Foo1(List<int> list)
{
    list.Add(1);
}

or this:

public List<int> Foo2()
{
    List<int> list = new List<int>();
    list.Add(1);
    return list;
}

The reason I ask is because I'm doing it the first way at the moment (except the method is different and more complicated obviously), which requires me to always use two lines to call the method as such (this adds up to a lot of extra lines):

List<int> list = new List<int>();
Foo1(list);

whereas with the second way I can just use one line:

List<int> list = Foo2();

So which way is better, with time and space in mind?

Edit: Okay so more specifically, I have a method that adds all the controls of type T from a ControlCollection to a List.

public static void GetControlsRec<T>(Control.ControlCollection controlCollection, List<T> resultCollection) where T : Control
{
    foreach (Control control in controlCollection)
    {
        if (control is T)
            resultCollection.Add((T)control);

        if (control.HasChildren)
            GetControlsRec(control.Controls, resultCollection);
    }
}

Which would be better in this case?

Answer 1

In general, I typically try to avoid changing/mutating an existing collection. As such, I'd almost always prefer your second option.

That being said, it does have the disadvantage of creating a new List<T>, which means more memory allocations. If (and only if) performance is an issue in that specific piece of code, you may want to consider modifying the input list directly, though I would suggest picking a method name which makes it obvious you're mutating the collection.

Answer 2

So the problem that you have is that you have a recursive method in which each call to the method conceptually adds some number of items to a result collection. This leads to two conceptual approaches, which you correctly identified:

1. Have each recursive call return a collection of all of the results that it represents. This requires each call to pull out the results of any recursive calls and add them to its own collection. This is rather inefficient and wasteful; you end up copying data around over and over again. (That is, unless you use a data structure that can efficiently "add all of the results from another instance of the same type". A LinkedList (that you rolled yourself, as the .NET version doesn't support this) could do this well, or some of the immutable data structures perhaps.)

2. Pass in a mutable collection type and have each recursive call mutate the collection. This will perform well, but leads to the problem that the code is hard to reason about. You can't just pull out one "sub tree" and look at that in isolation. It's also awkward for the caller in that they need to create a collection, leave it empty, store a reference to it so that they can access it after calling the method, etc. This is just very confusing and error prone.

option 1, as much as it makes things easier on the programmer, is really very wasteful (if you don't do some sort of optimization through the use of another type of collection, as described). If you do go with the section option I would strongly suggest abstracting that away from the caller. Specifically, make the overload accepting a List private, and have a separate public overload of the method without the extra parameter that passes in a list it creates to the private overload and then returns that list. This lets the caller think that you're using an approach similar to the the first method, while still getting the performance benefits of the second. It still does complicate development though.

The other option is to just avoid recursion entirely, which is my personal preference. When you solve the problem iteratively, not recursively, all of the problems just go away:

public static IEnumerable<Control> GetAllChildren(this Control root)
{
    var stack = new Stack<Control>();
    stack.Push(root);

    while (stack.Any())
    {
        var next = stack.Pop();
        foreach (Control child in next.Controls)
            stack.Push(child);
        yield return next;
    }
}

(If you want all controls of a particular type simply call OfType on the result of this query, it's really best to separate out the logical operations of "get all of the children" from "filter the collection to just these types of controls".)