Why Intellij Idea suggests to create intermediate List while converting array to Set using loop?

Question

Suppose I had the following code:

public Set<String> csvToSet(String src) {
    String[] splitted = src.split(",");
    Set<String> result = new HashSet<>(splitted.length);
    for (String s : splitted) {
        result.add(s);
    }
    return result;
}

so I need to transform an array into Set. And Intellij Idea suggests to replace my for-each loop with Collection.addAll one-liner so I get:

...
Set<String> result = new HashSet<>(splitted.length);
result.addAll(Arrays.asList(splitted));
return result;

The complete inspection message is:

This inspection warns when calling some method in a loop (e.g. collection.add(x)) could be replaced when calling a bulk method (e.g. collection.addAll(listOfX). If checkbox "Use Arrays.asList() to wrap arrays" is checked, the inspection will warn even if the original code iterates over an array while bulk method requires a Collection. In this case the quick-fix action will automatically wrap an array with Arrays.asList() call.

From inspection description it sounds like it works as expected.

If we refer to a top answer of a question about converting an array into Set (How to convert an Array to a Set in Java) the same one liner is suggested:

Set<T> mySet = new HashSet<T>(Arrays.asList(someArray));

Even though creating an ArrayList from array is O(1), I do not like the idea of creating an additional List object.

Usually I trust Intellij inspections and assume it does not offer anything less efficient. But today I am curious why both: top SO answer and Intellij Idea (with default settings) recommend using same one-liner with creating useless intermediate List object while there is also a Collections.addAll(destCollection, yourArray) since JDK 6.

The only reason I see for it is that both (inspection and answers) are too old. If so, here is the reason to improve intellij idea and give more votes to an answer proposing Collections.addAll() :)

Answer 1

A hint as to why Intellij doesn't suggest the Arrays.asList replacement for

Set<String> result = new HashSet<>(splitted.length);
result.addAll(Arrays.asList(splitted));
return result;

is in the source code for HashSet(Collection):

public HashSet(Collection<? extends E> c) {
    map = new HashMap<>(Math.max((int) (c.size()/.75f) + 1, 16));
    addAll(c);
}

Note that the capacity of the set isn't the size of c.

As such, the change would not be semantically equivalent.

---

Don't worry about creating the List. It is really cheap. It's not free; but you would have to be using it in a really performance-critical loop to notice.

Answer 2

I wrote a small function to measure the performance of the three ways of adding the array to a HashSet and here are the results.

First the base code used by all of them that would generate an array of maxSize with values between 0-100

    int maxSize = 10000000; // 10M values
    String[] s = new String[maxSize];
    Random r = new Random();

    for (int i = 0; i < maxSize; i++) {
        s[i] = "" + r.nextInt(100);
    }

Then the benchmark function:

public static void benchmark(String name, Runnable f) {
    Long startTime = System.nanoTime();
    f.run();
    Long endTime = System.nanoTime();
    System.out.println("Total execution time for: " + name + ": " + (endTime-startTime) / 1000000 + "ms");
}

So first way is using your code with a loop and for 10M values it took between 150ms and 190ms ( I ran the benchmark several times for each method)

    Main.benchmark("Normal loop ", () -> {
        Set<String> result = new HashSet<>(s.length);
        for (String a : s) {
            result.add(a);
        }
    });

Second is using result.addAll(Arrays.asList(s)); and it took between 180ms and 220ms

        Main.benchmark("result.addAll(Arrays.asList(s)): ", () -> {
            Set<String> result = new HashSet<>(s.length);
            result.addAll(Arrays.asList(s));
        });

Third way is using Collections.addAll(result, s); and it took between 170ms and 200ms

    Main.benchmark("Collections.addAll(result, s); ", () -> {
        Set<String> result = new HashSet<>(s.length);
        Collections.addAll(result, s);
    });

---

Now the explanation. From a runtime complexity they all run in O(n) meaning that for N values N operations are going to run (basically adding N values).

From a memory complexity point of view, is again, for all O(N). There's only the new HashSet which is created.

Arrays.asList(someArray) is not creating a new array, is just creating a new object that has a reference to that array. You can see it in the java code:

    private final E[] a;

    ArrayList(E[] array) {
        a = Objects.requireNonNull(array);
    }

Besides that, all the addAll methods are going to do exactly what you did, a for-loop:

// addAll method for Collections.addAll(result, s);
public static <T> boolean addAll(Collection<? super T> c, T... elements) {
    boolean result = false;
    for (T element : elements)
        result |= c.add(element);
    return result;
}

// addAll method for result.addAll(Arrays.asList(s));
public boolean addAll(Collection<? extends E> c) {
    boolean modified = false;
    for (E e : c)
        if (add(e))
            modified = true;
    return modified;
}

---

To conclude, the runtime difference being so small, IntelliJ suggests a way to write code in a more clear way and less code.