Are methods that modify reference type parameters bad?

Question

I've seen methods like this:

public void Foo(List<string> list)
{
    list.Add("Bar");
}

Is this good practice to modify parameters in a method?

Wouldn't this be better?

public List<string> Foo(List<string> list)
{
    // Edit
    List<string> newlist = new List<string>(list);
    newlist.Add("Bar");
    return newlist;
}

It just feels like the first example has unexpected side effects.

Answer 1

In the example you've given, the first seems a lot nicer to me than the second. If I saw a method that accepted a list and also returned a list, my first assumption would be that it was returning a new list and not touching the one it was given. The second method, therefore, is the one with unexpected side effects.

As long as your methods are named appropriately there's little danger in modifying the parameter. Consider this:

public void Fill<T>(IList<T> list)
{
    // add a bunch of items to list
}

With a name like "Fill" you can be pretty certain that the method will modify the list.

Answer 2

You're doing the exact same thing in both methods, just one of them is returning the same list.

It really depends on what you're doing, in my opinion. Just make sure your documentation is clear on what is going on. Write pre-conditions and post-conditions if you're into that sort of thing.

Answer 3

Frankly, in this case, both methods do more or less the same thing. Both will modify the List that was passed in.

If the objective is to have lists immutable by such a method, the second example should make a copy of the List that was sent in, and then perform the Add operation on the new List and then return that.

I'm not familiar with C# nor .NET, so my guess would be something along the line of:

public List<string> Foo(List<string> list)
{
    List<string> newList = (List<string>)list.Clone();
    newList.Add("Bar");
    return newList;
}

This way, the method which calls the Foo method will get the newly created List returned, and the original List that was passed in would not be touched.

This really is up to the "contract" of your specifications or API, so in cases where Lists can just be modified, I don't see a problem with going with the first approach.

Answer 4

The advent of extension methods has made it a bit easier to deal with methods that introduce side effects. For example, in your example it becomes much more intuitive to say

public static class Extensions
{
  public static void AddBar(this List<string> list)
  {
     list.Add("Bar");
  }
}

and call it with

mylist.AddBar();

which makes it clearer that something is happening to the list.

As mentioned in the comments, this is most useful on lists since modifications to a list can tend to be more confusing. On a simple object, I would tend to just to modify the object in place.

Answer 5

It's actually not that unexpected that a method that takes a list as parameter modifies the list. If you want a method that only reads from the list, you would use an interface that only allows reading:

public int GetLongest(IEnumerable<string> list) {
 int len = 0;
 foreach (string s in list) {
  len = Math.Max(len, s.Length);
 }
 return len;
}

By using an interface like this you don't only prohibit the method from changing the list, it also gets more flexible as it can use any collection that implements the interface, like a string array for example.

Some other languages has a const keyword that can be applied to parameters to prohibit a method from changing them. As .NET has interfaces that you can use for this and strings that are immutable, there isn't really a need for const parameters.