Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Below is my code that was written for the following task (based on an SO question):

Given a Java Collection (of N elements) create an ArrayList, containing N collections of the same type with just one element in each.

Here's my code for such method:

public static <E> ArrayList<Collection<E>> SplitCollection (Collection<E> col) throws InstantiationException, IllegalAccessException {
    ArrayList<Collection<E>> listOfCollections = new ArrayList<>();
    for(E el: col) {
        // creating an empty collection of type E
        Collection<E> colEl = col.getClass().newInstance();
        colEl.add(el);
        listOfCollections.add(colEl);
    }
    return listOfCollections;
}
  1. Is there a better way to create a run-time instance of an implementation class for an interface?
  2. Did I overdo anything in light of type erasure?
  3. Other code critique?

When I decided to use no-argument constructor I was aware that the very existence of one is not (and cannot be) enforced by the interface. As it stated in the docs:

All general-purpose Collection implementation classes (which typically implement Collection indirectly through one of its subinterfaces) should provide two "standard" constructors: a void (no arguments) constructor, which creates an empty collection, and a constructor with a single argument of type Collection, which creates a new collection with the same elements as its argument. In effect, the latter constructor allows the user to copy any collection, producing an equivalent collection of the desired implementation type. There is no way to enforce this convention (as interfaces cannot contain constructors) but all of the general-purpose Collection implementations in the Java platform libraries comply.

So I was writing my code for general-purpose Collection implementations.

I never intended this code to be used for production purposes. I merely wanted to see how far programming to interface together with Generics could get you before you have to use a concrete type and implementation.

share|improve this question

2 Answers 2

up vote 13 down vote accepted

Two parts to this review:

  1. the 1-liner for the newInstance()
  2. the general mechanism of the method

newInstance()

This code will work, but only for a subset of Collections. For example, there are many Collection implementations that do not have an accessible default constructor. What if the input collection is:

Collection<String> input = Arrays.asList(new String[]{"hello","world"});

The newInstance call on the class of that List will fail.... because that is a class called java.util.Arrays$ArrayList and it is not public at all...

The 'specification' for this problem is probably contrived, and there is no expectation that it should behave well when the inputs are bad. As a contrived example, I guess contrived code is OK.

Which leads on to the next thing, what is the right thing to do?

Well, I don't think there is a right thing to do. If the input requires the support of bare 'Collection' then you have no hope.

In a real example, I would probably specify the method to allow specific known collections that the program would expect. Then not use the newInstance at all.

Alternatively, I would use the Java-8 style Supplier, and have an input that supplies you with the collection you need, and shift the burden to the user of your method. Something like:

public interface CollectionSupplier<T> {
    Collection<T> supply();
}

and then have your method declared as:

public static <E> ArrayList<Collection<E>> SplitCollection (
        final Collection<E> col, final CollectionSupplier<E> supplier) {
    .....
    Collection<E> colEl = supplier.supply();
    ....
}

That would also remove the ugly exceptions you declare to throw... as it is, I would probably (bad practice) 'try' the whole loop, and 'catch' the ugly exceptions and replace them with a wrapper of an IllegalStateException that explains that an inner Collection could not be instantiated.

General

Overall your method is neat, and well structured. I can't really fault it, given the constraints of the problem.

Update: Actually, the method should have a lower-case 's' to start the name. How did I miss that the first time? Should be splitCollection not SplitCollection

I would prefer that the method declared the input col as final: public static <E> ArrayList<Collection<E>> SplitCollection (final Collection<E> col) because that is a habit I am in...

Typically I would recommend against returning the data typed as an ArrayList because the implementation should not be reflected, it should just be List, but, again, in this case, the specification requires an ArrayList return value.

Bonus

A Java 8 implementation of this would be a good exercise.

This is what I would try:

public static <T, U extends Collection<T>, V extends Collection<T>> ArrayList<V> splitCollection (U col, Supplier<V> supplier) {

    return col.stream().map(v -> {
           V subcol = supplier.get();
           subcol.add(v);
           return subcol;
        }).collect(Collectors.toCollection(ArrayList::new));
}

and it would be used like:

public static void main(String[] args) {
    System.out.println("Result:"
       + splitCollection(Arrays.asList("hello","world"), ArrayList::new));
}

which gives the output:

Result:[[hello], [world]]

Note, in the Java 8 example, I allow the sub collection to be any supported type that you can add the content to. So, for the above example, the input is an inaccessible list (no public default constructor), but the supplier gives you what you need.

share|improve this answer
    
Updated to have a Java8 example, and point out the lower-case S to start with –  rolfl Jul 24 at 3:03
    
You could make an overloaded version of splitCollection that wraps every element in an ArrayList, which is the default behaviour by many Java 8 libraries. It is good though that you still allow the user to pass in its own collection instance. +1 –  skiwi Jul 24 at 7:05

There is a philosophy issue with this problem. The whole point of the Collection interface is to act as a high level interface for Collections. It throws away the implementation specific detail by design. Trying to get it back will usually only introduce the exact problems that the Collection interface exists to solve.

Therefore you cannot reliably complete this question. The newInstance method will not always work, particularly if you start using it on homegrown collections. For example, the newInstance() method requires that the object has a blank constructor, you can easily write a collection for which this fails. If you try this on Googles Immutable Collections, then either this line, or when you attempt to add an object will fail (I don't actually know if it has a constructor, you are meant to use the factory or the builder).

In practice this code will usually work, but putting it into a production application invites a subtle bug when a client tries to use it on a collection which cannot be instantiated in this way.

share|improve this answer
    
My code was purely a (mental) exercise and was never meant for production purposes. –  PM 77-1 Jul 24 at 19:36

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Not the answer you're looking for? Browse other questions tagged or ask your own question.