Use of guava immutable collection as method parame

Question

I am trying to determine what the best practices would be for an ImmutableList. Below is a simplistic example that will help drive my questions:

Ex:

public ImmutableCollection<Foo> getFooOne(ImmutableList<Foo> fooInput){ 
   //.. do some work
   ImmutableList<Foo> fooOther = // something generated during the code
   return fooOther;
}

public Collection<Foo> getFooTwo(List<Foo> fooInput){
   //.. do some work
   List<Foo> fooOther = // something generated during the code
   return ImmutableList.copyOf(fooOther);
}

public void doSomethingOne(){
  ImmutableCollection<Foo> myFoo = getFooOne(myList);
  ...
  someOtherMethod(myFoo);
}

public void doSomethingTwo(){
  Collection<Foo> myFoo = getFooOne(myList);
  ...
  someOtherMethod(myFoo);
}

My Questions:

1. Which makes the most sense to use in an application? [doSomethingOne and getFooOne] or [doSomethingTwo and fooTwo]? In other words if you know you are using ImmutableCollections does it make sense to keep casting back and forth and doing copyOf(), or just use Immutable everywhere?

2. These examples are public methods which could imply that other people use them. Would any of these answers change if the methods were private and used internally?

3. If a user tries to add anything to an immutable List an exception will be thrown. Because they may not be aware of this, wouldn't it make more sense to explicitly return an ImmutableCollection instead of a Collection?

Answer 1

In general, it's wise not to commit to a specific implementation in your declared return type, but we think of the immutable types as an exception. There are a few reasons to declare a return type of Immutable*:

• They document that you're returning a snapshot, not a live view.
• They document that the caller can't mutate the result.
• They document that insertion order is preserved (which may or may not be significant in your use case).
• They document that the collection won't contain null.
• Someone might want the asList() or reverse() method.
• You may save someone a copyOf() call if he wishes to assign to an Immutable* field. (But note that, if he does include copyOf(), it will short-circuit for most immutable inputs, even if you don't declare the return type.)

Basically, I'm just cribbing from https://github.com/google/guava/wiki/TenThingsAboutImmutableCollections, which you may want to check out in its entirety.

Answer 2

public ImmutableCollection<Foo> getFooOne(ImmutableList<Foo> fooInput){
   ImmutableList<Foo> fooOther= fooInput;
   return ImmutableList.copyOf(fooOther);
}

This makes no sense at all. Why would you ever copy an immutable collection? The whole point of immutability is: it can't be changed, so you might as well re-use it.

public Collection<Foo> getFooTwo(List<Foo> fooInput){
   ImmutableList<Foo> fooOther= ImmutableList.copyOf(fooInput);
   return ImmutableList.copyOf(fooOther);
}

??? Why do it twice??? This is fine:

public Collection<Foo> getFooTwo(List<Foo> fooInput){
   return ImmutableList.copyOf(fooInput);
}

ImmutableList.copyOf(Collection) is smart enough to return ImmutableList unmodified and create a new ImmutableList for everything else.

Answer 3

My usual approach is:

• accept List for parameters (so the interface is easier to use for clients)

• if performance/memory usage/thread-safety is important, copy the contents of the provided List into a data structure that is optimized for usage by your class

• when returning an ImmutableList, ImmutableList should be the return type (because it gives the caller more information about how it can use the returned value)

• when returning a mutable implementation of List, List should be the return type, unless something else about the return type is important (thread-safety, as a bad* example)

* It's a bad example because if your return values need to be thread-safe, it probably means something else is wrong with your code.

Replace List/ImmutableList with any of the immutable collection types.

Answer 4

You should always use the standard JRE classes on public interfaces. There are no extra methods on Guava's Immutable... classes so you're not gaining any compile-time safety: any attempts to make changes to those objects will only fail at run-time (but see Bart's comment). You should document in methods that return collections that they're immutable.

You should make defensive copies of lists provided on public methods if you're worried about concurrent modification, but it's OK to specify ImmutableCollection on private method arguments.

Answer 5

If I understood your intentions, the proper way of designing getFooXxx for making an immutable copy of maybe-mutable-list is something like this:

/**
 * Returns an <b>immutable copy</b> of input list.
 */
public ImmutableList<Foo> immutableCopyOfFoo(List<Foo> input){
    return ImmutableList.copyOf(input);
}

Why?

• ImmutableList.copyOf() does it's magic when given list is immutable,
• method signature explicitly says what it does,
• method returns ImmutableList which is, in fact, an ImmutableCollection but why would you like to hide information about ImmutableList from user? If he wants, he'll write Iterable foo = immutableCopyOfFoo(mutableFoo); instead, but 99% he'll use an ImmtableList,
• returning an ImmutableList makes a promise - "I am immutable, and I will blow everything up if you try to change me!"

• and last but not least - proposed method is unnecessary in internal use; just use

  someOtherMethod(ImmutableList.copyOf(foo));
  

  directly in your code...

---

You should check @ChrisPovirk's answer (and link to wiki in that answer) to know that i.e. when List<Foo> input contains nulls, you will get nasty NPE on runtime if you try to make an immutable copy...

---

EDIT answering comment #1:

Collection contract is less strict than List's one; i.e. Collection doesn't guarantee any order of elements ("Some are ordered and others unordered") while List does ("An ordered collection (also known as a sequence)").

If an input is a List it suggests that order is important and therefore output should guarantee the same. Imagine that:

public ImmutableCollection<Foo> immutableCopyOfFoo(List<Foo> input){
    return ImmutableSortedSet.copyOf(input, someFancyComparator);
}

It doesn't smell right. If you don't care about order then maybe method signature should be immutableCopyOfFoo(Collection<Foo> input)? But it depends on concrete use case.