What good practice is changing a list in a method or returning a new list to a method?

Question

Code example:

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 third option that I believe: you can create a new List in the modifyMyList method and return this new list ...

(the third option is here, I was too lazy, but someone already added it to 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 a reason I should choose one by one? What should be considered in this case?

Answer 1

I have a rule (self-imposed) that is "Never mutate a method parameter in a public method." Thus, in a private method, it is normal to mutate a parameter (I am even trying to avoid this case too). But when calling the public method, parameters should never be mutated and should be considered immutable.

I think the arguments of the mutating method are a bit hacky and can lead to errors that are harder to see.

I know that I am making exceptions to this rule, but I need a really good reason.

Answer 2

In fact, there is no functional difference.

You will know the difference when you want to return the list.

 List someNewList = someInstnace.modifyMyList(list); 

Answer 3

I recommend creating a new list in the method and returning an immutable list. This way, your code will work even when you are transferred to the immutable list. As a rule, creating immutable objects is a good practice, as we usually move towards functional programming and try to scale across several 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 4

The second is probably confusing, as it implies that a new value is created and returned - and this is not so.

An exception would be if this method were part of a “smooth” API, where the method was an instance method and modifying its instance and then returning the instance to allow the method chain: Java class StringBuilder example of this.

In general, I would not use it either.

I would choose the third option: I am writing a method that creates and returns a new list with the corresponding change. This is a bit artificial in the case of your example, since the example really just plays 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; } 

In addition: I would not want, as Saket suggested, to return an immutable list. His argument for immutability and parallelism is valid. But most of the time, Java programmers expect to modify the collection, except in special circumstances. By providing you with a method of returning an immutable collection, you limit its reuse for such circumstances. (The caller can always make the list immutable if he wants: they know that the return value is a copy and will not be affected by anything else.) In other words: Java is not Clojure. Also, if parallelism is a concern, take a look at Java 8 and threads (the new type is not I / O threads).

Here is another 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 have generalized the method and made it more widely applicable for making Iterable instead of a list. In real code, I would have two overloaded versions, one would take Iterable and one a Iterator . (The first will be implemented by calling the second, with an iterative iterator.) In addition, I made it static, because there was no reason why your method would be an instance method (it does not depend on the state from the instance).

Sometimes, if I write library code, and if it is not clear whether it is more useful to use a mutating or non-mutating implementation, I create both. Here's a more complete 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 it were in a real API, I would check the arguments for null and throw an IllegalArgumentException. (NOT NullPointerException - although this is often used for this purpose. NullPointerException also occurs for other reasons, for example, with an error. IllegalArgumentException is better for invalid parameters.)

(There will also be more Javadoc than valid code!)

Answer 5

The first and second solutions are very similar. The advantage of the second is to allow the chain. The question of “is this good practice” is being debated, as we can see here: Method chain in Java

So the real question is the first solution with a mutable list, and the third one with an immutable list, and again, there is no single answer, these are the same disputes between returning String that are immutable and use Stringbuffer, which are mutable, but provide better performance .

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

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

Answer 6

Both methods will work, because in this case java works with the link in the List, but I prefer the seound path, because this solution also works for passing by value, and not just for passing by reference.

Answer 7

Functionally, both are the same.

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

While the first method would make it clear (of course, based on the method naming convention) that it would change the original list (the same object).

In addition, the second method returns a list, so ideally the caller should check for a null return value, even if the passed list is not equal to zero (the method can potentially return zero instead of the modified list).

Given this, I usually prefer to use the method once per second.