0

I would like to concatenate two array lists as it shows in the answer:

final List<String> l1 = Lists.newArrayList(...);
final List<String> l2 = Lists.newArrayList(...);
List<String> l = new ArrayList<String>() { { addAll(l1); addAll(l2); } };

Is there any non style reason I should not use it this way?

Community
  • 1
  • 1
Loom
  • 9,042
  • 19
  • 50
  • 102
  • I think this is primary opinion-based. – Konstantin Yovkov Jun 15 '15 at 13:07
  • 2
    You are creating an anonymous inner class each time you do that. That may lead to GC pressure and have issues with serialization. – Todd Jun 15 '15 at 13:09
  • @Todd - Thank you! So, if it is the only operation with short `l1`, `l2` it is ok to use this construction? – Loom Jun 15 '15 at 13:15
  • 1
    As with all anonymous non-static inner class there is a reference to the enclosing outer class. GC won't be able to clean up the list until the outer class is able to be cleaned up. If this piece of code is in a method run heavily then there will be a significant memory leak. Please don't use this horrible piece of syntax. – Brett Walker Jun 15 '15 at 13:18

2 Answers2

2

If you are already using Guava, you can do this using the following one-liner:

List<String> l = FluentIterable.from(l1).append(l2).copyInto(new ArrayList<String>());

I personally don't like double braces. This produces new class just for the purpose to initialize it in different way. New class means bigger target jar file, more memory is necessary for JVM to store classes, bigger inheritance table for JIT compiler, etc. This can become significant if you use such approach everywhere. If you return this ArrayList to another component and your current component is not used anymore (for example, you are using OSGi or other module system), you may end up having hanging ClassLoader which is linked only by this anonymous class. Also if you care about serialization, you should declare serialVersionID in this new class. Even if you don't care, you may have a warning about this.

Tagir Valeev
  • 87,515
  • 18
  • 194
  • 305
1

Well, first of all, it depends on what you wish to do with duplicate elements. This code will keep any duplicates (i.e. if l1 and l2 both contain "someString", l will contain two "someString"), so it's not really a union operation. Calling it a concatenation of two lists would be more accurate.

Other than that, using the "double brace" is actually creating an instance of an anonymous class that extends ArrayList. I think it's a bad code style.

List<String> l = new ArrayList<String>(l1);
l.addAll(l2);

is clearer and is not longer to write.

Eran
  • 359,724
  • 45
  • 626
  • 694
  • Thank you. (+1) I corrected the quesion. I am novice. Why it is a bad code style to create an instance of anonymous class that extends `ArrayList`? – Loom Jun 15 '15 at 13:22
  • @Loom I consider it a bad style because you don't need a sub-class of ArrayList in this use case (since you are not adding any new behavior on top of the ArrayList behavior). You need an instance of ArrayList and you create an anonymous sub-class instance for no reason. – Eran Jun 15 '15 at 14:17