CODEWITHSUNDEEP
javarefactoring
Selbi
Selbi

Reputation: 833

How do I refactor this loop?

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?

Upvotes: 46

Views: 5104

Answers (4)

Andy Turner
Andy Turner

Reputation: 140309

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.

Upvotes: 70

Roland
Roland

Reputation: 23242

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.

Upvotes: 17

eve2017
eve2017

Reputation: 11

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.

Upvotes: 1

jack jay
jack jay

Reputation: 2503

You should achieve this by passing List after converting it to array.

Retain this as your single method,

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

and when you want to pass a List<Item> then pass like this,

something(listItem.toArray(new Item[listItem.size()]))

Upvotes: 0

Related Questions