Tidying up the loose ends
Some of you might have been a little incredulous at my earlier statement that no existing library offers methods to neatly deal with possibly-null varargs.
I stand by my position - it's the possibly-null aspect that is the kicker. Google Guava offers methods that almost get over the line:
| My varargs method | Google's almost-the-same method |
|---|
Iterable<T> VarargIterator.forArgs
(T... optionalArgs) |
ArrayList<E> Lists.newArrayList
(E... elements) |
| Iterable<T> VarargIterator.forCompulsoryAndOptionalArgs(T t, T... ts) |
List<E> Lists.asList
(E first,
E[] rest) |
BUT they explode when fed nulls - making them pointless for this particular wart-removing exercise!
So the only thing left now is to clean up the implementation a little, because I think it's failing two of the rules of clean code:
- Runs all the tests
- Contains no duplications
- Expresses the intent of the programmers
- Minimizes the number of classes and methods
Here's the entire class as it stands (imports removed):
public class VarargIterator {
/**
* @return an {@link Iterable} of a suitable type.
* Null-safe; passing a null varargs will simply return
* an "empty" {@code Iterable}.
*/
public static final <T> Iterable<T> forArgs(T... optionalArgs) {
if (optionalArgs == null) {
return Collections.emptyList();
} else {
return Arrays.asList(optionalArgs);
}
}
/**
* @return an {@link Iterable} of a suitable type.
* Null-safe; passing a null {@code optionalArgs} will simply return
* a single-element {@code Iterable}.
* @throws IllegalArgumentException if {@code compulsoryArg} is null
*/
public static final <T> Iterable<T> forCompulsoryAndOptionalArgs(
final T compulsoryArg, final T... optionalArgs) {
Validate.notNull(compulsoryArg,
"the first argument to this method is compulsory");
if (optionalArgs == null) {
return Collections.singletonList(compulsoryArg);
} else {
List<T> iterable = new ArrayList<T>(optionalArgs.length + 1);
iterable.add(compulsoryArg);
iterable.addAll(Arrays.asList(>optionalArgs));
return iterable;
}
}
}
The bits that I don't like are the repetition of the optionalArgs null check in both methods, and the code in the else case of the second method. It's working at a "different level" to the other code in the method, losing the intent, and there's too much of it - it's made the whole method too long.
Of course I have a full suite of unit tests so I can be confident that I'm not breaking anything when I do this refactoring work. I use Cobertura in its Maven and Eclipse plugin forms to ensure I'm achieving 100% coverage from these tests.
The first step is easy - extract out a well-named method for the else case:
/**
* @return an {@link Iterable} of a suitable type.
* Null-safe; passing a null {@code optionalArgs} will simply return
* a single-element {@code Iterable}.
* @throws IllegalArgumentException if {@code compulsoryArg} is null
*/
public static final <T> Iterable<T> forCompulsoryAndOptionalArgs(
T compulsoryArg, T... optionalArgs) {
Validate.notNull(compulsoryArg,
"the first argument to this method is compulsory");
if (optionalArgs == null) {
return Collections.singletonList(compulsoryArg);
} else {
return createListFrom(compulsoryArg, optionalArgs);
}
}
private static <T> Iterable<T> createListFrom(
T compulsoryArg, T... optionalArgs) {
final List<T> list = new ArrayList<T>(optionalArgs.length + 1);
list.add(compulsoryArg);
list.addAll(Arrays.asList(optionalArgs));
return list;
}
I dithered a bit about the null check. Doing some kind of is-null/is-not-null closurey-interfacey thing seemed like pretty major overkill, so in the end I just extracted out the logic into a method. As a bonus, I realised I could also check for a zero-length (as opposed to null) array and save some cycles in that case. One last tweak was to drop the explicit else branches - because the methods are now so short there seems little point. So here's the final version - enjoy!
public class VarargIterator {
/**
* @return an {@link Iterable} of a suitable type.
* Null-safe; passing a null varargs will simply return
* an "empty" {@code Iterable}.
*/
public static final <T> Iterable<T> forArgs(T... optionalArgs) {
if (isEmpty(optionalArgs)) {
return Collections.emptyList();
}
return Arrays.asList(optionalArgs);
}
/**
* @return an {@link Iterable} of a suitable type.
* Null-safe; passing a null {@code optionalArgs} will simply return
* a single-element {@code Iterable}.
* @throws IllegalArgumentException if {@code compulsoryArg} is null
*/
public static final <T> Iterable<T> forCompulsoryAndOptionalArgs(
T compulsoryArg, T... optionalArgs) {
Validate.notNull(compulsoryArg,
"the first argument to this method is compulsory");
if (isEmpty(optionalArgs)) {
return Collections.singletonList(compulsoryArg);
}
return createListFrom(compulsoryArg, optionalArgs);
}
private static boolean isEmpty(Object... optionalArgs) {
return (optionalArgs == null) || (optionalArgs.length == 0);
}
private static <T> Iterable<T> createListFrom(
T compulsoryArg, T... optionalArgs) {
final List<T> list = new ArrayList<T>(optionalArgs.length + 1);
list.add(compulsoryArg);
list.addAll(Arrays.asList(optionalArgs));
return list;
}
}
