How do I refactor this loop?

Question

I've got an application where I use primitive arrays and Lists for a class called Item. These are used interchangeably for legacy reasons (I also wish this was just one type, but it's the way it is).

Now I have to add a new method like this that works via a for-each loop:

public void something(Item... items) {     for (Item i : items) {         doStuff();     } }  public void something(List<Item> items) {     for (Item i : items) {         doStuff();     } } 

In other words, exactly the same method twice for both primitive Arrays and Lists. Is there any way to nicely refactor this into a single method?

Answer 1

You can't shouldn't (*) do this in a single method. Item[] and List<Item> are unrelated types.

You should make one of the overloads call the other: either something(Item... items) calls something(List<Item>), or something(List<Item>) calls something(Item... items).

Of the two options, it is better for the array overload to call the list overload:

public void something(Item... items) {   something(Arrays.asList(item)); } 

This is cheap, because it doesn't copy the array, but rather wraps it: creating the List is O(1).

If you were to invoke the array overload from the list overload:

public void something(List<Item> items) {   something(items.toArray(new Item[0])); } 

This would be more expensive, since the toArray call has to create and populate an array: it is an O(n) operation, where n is the size of the list. However, it has the slight advantage that something would not be able to replace the contents of the List, since any updates to the array are simply discarded after execution.

---

(*) You can, but it would be really gross, and not type-safe, as you'd have to accept an Object parameter, as there is no other common super type of List<Item> and Item[]; and you'd still end up having to repeat the loops for the two types; and you'd have to handle the possibility of a completely unrelated type being passed in (at runtime):

public void something(Object obj) {   if (obj instanceof List) {     for (Object element : (List<?>) obj) {       Item item = (Item) element;  // Potential ClassCastException.       doStuff();     }   } else if (obj instanceof Item[]) {     for (Item item : (Item[]) obj) {       doStuff();     }   } else {     throw new IllegalArgumentException();   } } 

What a mess. Thank the maker for overloads.

Answer 2

If you use Java 8 you could also just call forEach or map on your Stream and you are done, e.g.

yourStream.forEach(doStuff()); 

where doStuff() is the consumer dealing with the String or use yourStream.forEach(s -> doStuff()) if you don't want to handle the string and just do stuff.

You can obtain a stream as follows:

Stream.of(yourArray) // or Arrays.stream(yourArray)       .forEach(doStuff()); 

and for your list:

list.stream()     .forEach(doStuff()); 

The main benefit of using the streams is probably readability. It may lose regarding performance and it may also lose if you don't want to call Stream.of/Arrays.stream or Collection.stream() just to obtain the stream.

If you really want to keep the something(...) method (being able to deal with both: the varargs and the list) you are still requiring an overloaded method or use Andy Turner's proposal with the Object-parameter-method.