Tidier Varargs, part 3

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:

1. Runs all the tests
2. Contains no duplications
3. Expresses the intent of the programmers
4. 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;
    }
}