-1

So I've been looking back on code I've written almost a year and a half ago trying to fix it and I found this function that has got me confused. I searched for a method to do the same thing and found a pretty decent function and I'm curious at what would be better.

Function A:

public static String listToString(List<String> list){
    StringBuilder sb = new StringBuilder();
    if (list != null && list.size() > 1){
        sb.append(list.get(0));
        for (int i = 1; i < list.size(); i++){
            sb.append(", " + list.get(i));
        }
    }else if(list != null && (list.size() == 1)){
        sb.append(list.get(0));
    }
    return sb.toString();
}

Function B:

public static String listToString(List<String> list) {
    String str = "";
    for (String s : list) {
        str += s + ", ";
    }
    return str;
}

Now I wrote Function A within my first couple months of learning Java so I probably didn't know best though is there any reason I should stick to this method?

Spedwards
  • 4,167
  • 16
  • 49
  • 106
  • 3
    http://docs.guava-libraries.googlecode.com/git/javadoc/com/google/common/base/Joiner.html – Matt Ball Nov 14 '14 at 02:38
  • Function B is bad, as `String` concatenation in loops can't be optimised by the JVM at runtime and is woefully inefficient (creating lots of short lived objects that need to be GC). Function A is in the right direction, except `sb.append(", " + list.get(i));` defeats the purpose and should be `sb.append(", ").append(list.get(i));` – MadProgrammer Nov 14 '14 at 02:38
  • Apache Commons has `StringUtils.join`. Don't re-invent wheels. – Dawood ibn Kareem Nov 14 '14 at 02:45
  • @MadProgrammer The main problem isn't the object allocation, but the fact that `str3 = str1 + str2` requires copying all of the chars in `str1` and `str2` into the `char[]` that `str3` uses (twice, actually: once for the temporary `StringBuilder` and once for the new `String` it produces). If you do `str += foo`, that means that each time around, the previous `str` chars will be copied to create the new `str`. The original str's chars are copied N times, the first loop's str chars are copied N-1 times, etc. It's an N^2 operation, which is worse than the object allocation. – yshavit Nov 14 '14 at 03:58
  • @yshavit I was under the impression that String consternation in a loop didn't involve a StringBuilder ... Haven't really looked on to it do I could be over simplifying – MadProgrammer Nov 14 '14 at 04:01

4 Answers4

1

Function A is preferable since it is using only one instance of StringBuilder while code from Function B executed in loop

str += s + ", ";

is equivalent of

str = new StringBuilder(str).append(s).append(", ").toString();

so in each iteration you have to:

  • create new StringBuilder
  • copy content of current string to this StringBuilder
  • now you can add ", " and s
  • create and return new String based on current content of StringBuilder

(so in each iteration your need to read entire string again, and again, and again...) while in scenario B you just focus on adding new characters to single StringBuilder


Anyway since Java 8 you don't need to focus this much on writing methods which can concatenate collection of strings. You can simply use StringJoiner class or simpler just invoke
join(CharSequence delimiter, Iterable<? extends CharSequence> elements) method from String class which will use StringJoiner for you. Code of this method looks like:

public static String join(CharSequence delimiter,
        Iterable<? extends CharSequence> elements) {
    Objects.requireNonNull(delimiter);
    Objects.requireNonNull(elements);
    StringJoiner joiner = new StringJoiner(delimiter);
    for (CharSequence cs: elements) {
        joiner.add(cs);
    }
    return joiner.toString();
}

Note that Objects.requireNonNull(delimiter); will throw NPE in case of null, so if you want to avoid it you can write your own version of this method and replace this tests with something more like

if (delimiter==null || elements==null) return "";

or if you would like to allow delimiter to be null

if (elements==null) return "";
if (delimiter == null) delimiter = "";//use empty string as delimiter 
Pshemo
  • 122,468
  • 25
  • 185
  • 269
1

Apache's commons lang, just appends

 StringUtils.join(list);

or separate by characters.

StringUtils.join(java.lang.Iterable,char)

Java 8.0 onwards, first argument is to separate elements

String joined = String.join("", list);
Ankur Singhal
  • 26,012
  • 16
  • 82
  • 116
1

check Joiner on guava

    String s= Joiner.on(",").join(lists);

Check below output test

    List<String> lists=new ArrayList<String>();
    lists.add("a1");
    lists.add("a2");
    lists.add("a3");
    lists.add("a4");
    String s = Joiner.on(",").join(lists);
    System.out.println(s);

ouput:

a1,a2,a3,a4
JaskeyLam
  • 15,405
  • 21
  • 114
  • 149
0

Besides using libraries you could use

(the pattern I've seen most often)

public static String listToString(List<String> list) {
    StringBuilder sb = new StringBuilder();
    boolean first = true;
    for (String string : list) {
        if (first)
            first = false;
        else
            sb.append(", ");
        sb.append(string);
    }
    return sb.toString();
}

(also quite common)

public static String listToString(List<String> list) {
    StringBuilder sb = new StringBuilder();
    String separator = "";
    for (String string : list) {
        sb.append(separator).append(string);
        separator = ", ";
    }
    return sb.toString();
}

(shortest, bit hard to read)

public static String listToString(List<String> list) {
    StringBuilder sb = new StringBuilder();
    for (Iterator<String> iterator = list.iterator(); iterator.hasNext();)
        sb.append(iterator.next()).append(iterator.hasNext() ? ", " : "");
    return sb.toString();
}

(could be the fastest since no extra conditional jumps in the loops)

public static String listToString(List<String> list) {
    StringBuilder sb = new StringBuilder();
    Iterator<String> iterator = list.iterator();
    if (iterator.hasNext()) {
        sb.append(iterator.next());
        while (iterator.hasNext())
            sb.append(", ").append(iterator.next());
    }
    return sb.toString();
}

Or something else that is readable & short like B but uses a StringBuilder. What you don't want to have is + string concatenation in a loop. That creates temporary objects.

zapl
  • 63,179
  • 10
  • 123
  • 154