How to reorganize this cycle?

I have an application in which I use primitive arrays and lists for a class called Item. They are used interchangeably for hereditary reasons (I also want this to be only one type, but the way it is).

Now I have to add a new method like this, which works through a for loop: each

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 primitive arrays and lists. Is there a way to reorganize this well into one method?

+46
java refactoring
Jan 27 '17 at 15:11
source share
4 answers

You must not (*) do this in one method. Item[] and List<Item> are unrelated types.

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

Of the two parameters, it is better to overload the array to cause the list to be overloaded:

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

This is cheap because it does not copy the array, but rather wraps it: creating a List is O(1) .

If you were to call array overload from list overload:

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

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




(*) You can, but it would be very rude, and not safe, since you would need to accept the Object parameter, since there is no other common super-type List<Item> and Item[] ; and you still have to repeat the loops for the two types; and you have to handle the ability to transfer a completely unrelated type (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 creator for the overload.

+67
Jan 27 '17 at 15:14
source share

If you use Java 8, you can also just call forEach or map on Stream , and everything will be ready, for example

 yourStream.forEach(doStuff()); 

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

You can get the stream as follows:

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

and for your list:

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

The main advantage of using streams is probably readability. It may lose performance and may also lose if you do not want to call Stream.of/Arrays.stream or Collection.stream() just to receive the stream.

If you really want to save the something(...) method (the ability to deal with both: varargs and the list), you still need an overloaded method or use Andy Turner's suggestion using the Object parameter of the method.

+15
Jan 27 '17 at 15:28
source share

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

Also, if the list of items contains multiple items, you can use 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.

+1
Jan 27 '17 at 15:27
source share

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

Save this as your only method,

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

and if you want to pass List<Item> , then go this way

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

0
Jan 27 '17 at 15:26
source share



All Articles