current community

your communities

more stack exchange communities

Stack Exchange
tour help
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.
up vote 10 down vote favorite

I have written a few simple-to-be-used methods in Java 8, and am wondering what could be improved upon those:

public static <E, R, C extends Collection<? extends R>> C transform(final Collection<E> collection, final Function<? super E, ? extends R> mapper) {
    try {
        Objects.requireNonNull(collection);
        Objects.requireNonNull(mapper);
        Class<? extends Collection> clazz = collection.getClass();
        @SuppressWarnings("unchecked") Collection<Object> returnCollection = clazz.newInstance();
        collection.stream().map(mapper).forEach(returnCollection::add);
        @SuppressWarnings("unchecked") C genericReturnCollection = (C)returnCollection;
        return genericReturnCollection;
    } catch (InstantiationException | IllegalAccessException ex) {
        throw new RuntimeException(ex);
    }
}

public static <E, R, C extends Collection<R>> C transform(final Collection<E> collection, final Function<? super E, ? extends R> mapper, final Supplier<C> collectionSupplier) {
    Objects.requireNonNull(collection);
    Objects.requireNonNull(mapper);
    Objects.requireNonNull(collectionSupplier);
    return collection.stream().map(mapper).collect(Collectors.toCollection(collectionSupplier));
}

Their purpose is to provide similar functionality as the List.replaceAll() method, but then allowing that the mapped type differs from the original type. Hence it is something that should be in a static factory class, as it will always need to create a new collection.

Some example code:

LinkedList<Integer> linkedList = new LinkedList<>();
linkedList.add(5);
linkedList.add(10);
linkedList.add(15);
LinkedList<String> resultList = transform(linkedList, i -> (i + 1) + "-" + i);

and

LinkedList<Integer> linkedList = new LinkedList<>();
linkedList.add(5);
linkedList.add(10);
linkedList.add(15);
LinkedList<String> resultList = transform(linkedList, i -> (i + 1) + "-" + i, LinkedList::new);

Both have as output:

6-5
11-10
16-15

I am aware that the transform with a Supplier<C> as argument is obviously much nicer, however for convienience the single-argument version with only a function is much better.

share|improve this question

1 Answer 1

up vote 4 down vote accepted

I see the Supplier-less option as being a very broken one. Consider the following code:

transform(linkedList.sublist(0, linkedList.size() / 2), i -> (i + 1) + "-" + i);

Or, any of the other collections that do not have a meaningful default constructor:

transform(Arrays.asList(argv), a -> a.toLowerCase());

The code will cause more headaches than the small requirement to add the supplier will require.

The Generics of the mapping are also broken when there is no supplier, because the actual type of the return list should be more specific....

Lose the first static method.

The second method looks fine, except for the ordering of the arguments. I would make the supplier the middle, not the last argument (and I would introduce some newlines to make it not-so-long-of-a-line):

public static <E, R, C extends Collection<R>> C transform(
          final Collection<E> collection,
          final Supplier<C> collectionSupplier,
          final Function<? super E, ? extends R> mapper) {
    ....
}

The logical reading of that order is better, take this collection, map to that collection, and use this function to do it..... maybe it is not such a big deal...?

The use-case seems to read better for me:

List<String> lcargs = transform(Arrays.asList(args), ArrayList::new, a -> a.toLowerCase());
share|improve this answer

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.