How do I refactor this loop?

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.

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.

You can implement a single method, in this case, the second one because it has a list as parameter. Instead of the first method, you can convert the array in a list with Arrays.asList(items) and then, you can call the first method. So, in the end, you will have only a single method (that has a list as parameter).

Also, if the items list has few elements, you can use the lambda expressions from Java 8:

items.foreach(item -> doStuff(item));

So, you will not have a method that contains only one loop and the code will be easier to read.

How do I refactor this loop?

Related

Recent Posts