Which is good practice - Modifying a List in the method, or returning a new List in the method?

Question

Example code:

modifyMyList(myList);

public void modifyMyList(List someList){
    someList.add(someObject);
}

or:

List myList = modifyMyList(myList);

public List modifyMyList(List someList){
    someList.add(someObject)
    return someList;
}

There is also a 3rd option I believe: You can create a new List in modifyMyList method and return this new List...

( 3rd option is here, I was too lazy but someone already added it in the answers: )

List myList = modifyMyList(myList);

public List modifyMyList(List someList){
    List returnList = new ArrayList();
    returnList.addAll(someList);
    returnList.add(someObject);
    return Collections.unmodifiableList(returnList);
}

Is there any reason why I should choose one over another? What should be considered in such case?

Answer 1

As I said in my other answer, I don't think you should mutate the list parameter. But there are times where you also don't want to take a copy of the original list and mutate the copy.

• The original list might be large so the copy is expensive
• You want the copy to be kept up-to-date with any updates to the original list.

In these scenarios, you could create a MergedList which is a view over two (or perhaps more) lists

import java.util.*;

public class MergedList<T> extends AbstractList<T> {
    private final List<T> list1;
    private final List<T> list2;

    public MergedList(List<T> list1, List<T> list2) {
        this.list1 = list1;
        this.list2 = list2;
    }

    @Override
    public Iterator<T> iterator() {
        return new Iterator<T>() {
            Iterator<T> it1 = list1.iterator();
            Iterator<T> it2 = list1.iterator();

            @Override
            public boolean hasNext() {
                return it1.hasNext() || it2.hasNext();
            }

            @Override
            public T next() {
                return it1.hasNext() ? it1.next() : it2.next();
            }
        };
    }

    @Override
    public T get(int index) {
        int size1 = list1.size();
        return index < size1 ? list1.get(index) : list2.get(index - size1);
    }

    @Override
    public int size() {
        return list1.size() + list2.size();
    }
}

The you could do

public List<String> modifyMyList(List<String> someList){
    return new MergedList(someList, List.of("foo", "bar", "baz"));
}

Answer 2

The first and second solution are very similar, The advantage of the second is to permit chaining. The question of "is it a good practise" is subjected to debate as we can see here: Method Chaining in Java

So the real question is between the first solution with mutable list and the third with a unmutable list, and again, there is not a unique response, it is the same debate between returning String, which are immutable and using Stringbuffer, which are mutable but permits better performance.

If you need reliablility of your API , and if you don't have performance issues use immutable (the third solution). Use it if your lists are always small.

If you need only performance use a mutable list (the first solution)

Answer 3

The second is probably confusing as it implies a new value is being created and returned - and it isn't.

An exception would be if the method was part of a 'fluent' API, where the method was an instance method and was modifying its instance, and then returning the instance to allow method chaining: the Java StringBuilder class is an example of this.

In general, however, I wouldn't use either.

I'd go for your third option: I write a method that creates and returns a new list with the appropriate change. This is a bit artificial in the case of your example, as the example is really just reproducing List.add(), but...

/** Creates a copy of the list, with val appended. */
public static <T> List<T> modifyMyList(List<T> list, T val) {
    List<T> xs = new ArrayList<T>(list);
    xs.add(val);
    return xs;
}

Aside: I wouldn't, as suggested by Saket return an immutable list. His argument for immutability and parallelism is valid. But most of the time Java programmers expect to be able to modify a collection, except in special circumstances. By making you method return an immutable collection, you limit it's reusability to such circumstances. (The caller can always make the list immutable if they want to: they know the returned value is a copy and won't be touched by anything else.) Put another way: Java is not Clojure. Also, if parallelism is a concern, look at Java 8 and streams (the new kind - not I/O streams).

Here's a different example:

/** Returns a copy of a list sans-nulls. */
public static <T> List<T> compact(Iterable<T> it) {
    List<T> xs = new ArrayList<T>();
    for(T x : it)
        if(x!=null) xs.add(x);
    return xs;
}

Note that I've genercized the method and made it more widely applicable to taking an Iterable instead of a list. In real code, I'd have two overloaded versions, one taking an Iterable and one an Iterator. (The first would be implemented by calling the second, with the iterable's iterator.) Also, I've made it static as there was no reason for your method to be an instance method (it does not depend on state from the instance).

Sometimes, though, if I'm writing library code, and if it is not clear whether a mutating or non-mutating implementation is more generally useful, I create both. Here's a fuller example:

/** Returns a copy of the elements from an Iterable, as a List, sans-nulls. */ 
public static <T> List<T> compact(Iterable<T> it) {
    return compact(it.iterator());
}
public static <T> List<T> compact(Iterator<T> iter) {
    List<T> xs = new ArrayList<T>();
    while(iter.hasNext()) {
        T x = iter.next();
        if(x!=null) xs.add(x);
    }
    return xs;
}

/** In-place, mutating version of compact(). */
public static <T> void compactIn(Iterable<T> it) {
    // Note: for a 'fluent' version of this API, have this return 'it'.
    compactIn(it.iterator());
}
public static <T> void compactIn(Iterator<T> iter) {
    while(iter.hasNext()) {
        T x = iter.next();
        if(x==null) iter.remove();
    }
}

If this was in a real API I'd check the arguments for null and throw IllegalArgumentException. (NOT NullPointerException - though it is often used for this purpose. NullPointerException happens for other reasons as well, e.g. buggy code. IllegalArgumentException is better for invalid parameters.)

(There'd also be more Javadoc than actual code too!)

Answer 4

I have a (self imposed) rule which is "Never mutate a method parameter in a public method". So, in a private method, it's ok to mutate a parameter (I even try to avoid this case too). But when calling a public method, the parameters should never be mutated and should be considered immutable.

I think that mutating method arguments is a bit hacky and can lead to bugs that are harder to see.

I have been known to make exceptions to this rule but I need a really good reason.

Answer 5

Functionally both are same.

However when you expose your method as an API, second method may give an impression that it returns a new modified list other than the original passed list.

While the first method would make it clear (of-course based on method naming convention) that it will modify the original list (Same object).

Also, the second method returns a list, so ideally the caller should check for a null return value even if the passed list is non null (The method can potentially return a null instead of modified list).

Considering this I generally prefer to use method one over second.

Answer 6

I will recommend creating a new list in the method and returning an immutable list. That way your code will work even when you are passed in an Immutable list. It is generally a good practice to create immutable objects as we generally move towards functional programming and try to scale across multiple processor architectures.

List myList = modifyMyList(myList);

public List modifyMyList(List someList){
    List returnList = new ArrayList();
    returnList.addAll(someList);
    returnList.add(someObject);
    return Collections.unmodifiableList(returnList);
}

Answer 7

Actually there is no functional difference.

You'll come to know the difference when you want the returned list

List someNewList = someInstnace.modifyMyList(list);

Answer 8

Both ways will work because in this case java works with the reference of the List but i prefer the secound way because this solution works for pass by value too, not only for pass by reference.