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

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".)

Answer 2

In the generic case, there is no one blanket answer IMHO. It really depends on the context and circumstances.

In this specific case, I would probably return an IEnumerable instead and let the caller decide to put it in a list or not:

public static IEnumerable<T> GetControlsRec<T>(Control.ControlCollection controlCollection)
    where T : Control
{
    foreach (Control control in controlCollection)
    {
        if (control is T)
            yield return (T)control;

        if (control.HasChildren)
            foreach (T child in GetControlsRec(control.Controls))
                yield return child;
    }
}

---

Or better yet, slap an extension method onto Control itself, something akin to:

public static IEnumerable<T> GetDecendentsOfType<T>(this Control c)
{
    foreach (Control control in c.Controls)
    {
        if (control is T)
            yield return (T)control;

        if (control.HasChildren)
            foreach (T child in control.GetDecendentsOfType<T>())
                yield return child;
    }
}

which could then simply be called like:

Control myControl = Master.MakeMeAControl();
List<CheckBox> allCheckBoxesInControl = myControl.GetDecendentsOfType<CheckBox>().ToList();

Answer 3

Given your extra information I think the way you currently have it is correct. Since your using recursion to walk a tree you can keep passing down the list. If you didn't pass the list in then you'd have to create it (once for each level of recursion).

public static List<T> resultCollection GetControlsRec<T>(Control.ControlCollection controlCollection) where T : Control
{
    ///Create new collection
    List<T> resultCollection = new List<T>();
    foreach (Control control in controlCollection)
    {
        if (control is T)
            resultCollection.Add((T)control);

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

    return resultCollection;
}

Answer 4

In your specific case, I would recommend a generator:

public static IEnumerable<T> GetControlsRec<T>(Control.ControlCollection controlCollection) where T : Control
{
    foreach (Control control in controlCollection)
    {
        if (control is T)
           yield return (T)control;

        if (control.HasChildren)
            foreach (T descendant in GetControlsRec(control.Controls))
                yield return descendant;
    }
}

and then:

list.AddRange(GetControlsRec<…>(…));

(Sorry if the syntax isn’t quite right, but you get the idea.)

Answer 5

There is no [detectable] difference between

List<int> list = new List<int>();
Foo(list);
.
.
.
void Foo( List<int> c )
{
   c.Add(1) ;
   return ;
}

and

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

The first does a little work not done by the second in passing the parameter on the call to Foo(); the second does a little work not done by the first in returning the List<int> from the call to Foo(). Outside of that, there's essentially no difference.

Figure out the syntax you want to use (and one that makes sense for your design) and use that. Almost always, clarity and maintainability trump anything else. It's exceedingly unlikely that something like this would have any impact on performance.

And even easier, you could simply get rid of Foo() altogether. Something like:

List<int> c = Enumerable.Range(1,1).ToList() ;

or

List<int> c = new List<int>( new int[]{1} ) ;

should do you.

Answer 6

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 7

What I generally do is

public IList<int> Foo1(IList<int> list)
{
    list.Add(1);
    return list;
}

For me it's more readable because you identify clearly the input and the output. And because objects are passed by reference the performance is the same.

Hope it helps ^^