tags:

views:

1451

answers:

11

I am thinking of doing this way:


    List sList = new ArrayList();

    // Add elements.

    if (sList != null)
    {
        String listString = sList.toString;
        listString = listString.subString(1, listString.length() - 1);
    }

I somehow found this to be neater than using the StringBuilder/StringBuffer approach.

Any thoughts/comments?

+2  A: 

I somehow found this to be neater than using the StringBuilder/StringBuffer approach.

I guess it depends on what approach you took.

The AbstractCollection#toString() method simply iterates over all the elements and appends them to a StringBuilder. So your method may be saving a few lines of code but at the cost of extra String manipulation. Whether that tradeoff is a good one is up to you.

Kevin
The extra String manipulation you are talking about is creating subString at the end, right?
Jagmal
@Jagmal, yes. If this code gets run a lot, then you should profile to see if its a problem for you. Otherwise you should be OK. One thing to keep in mind is that the format of the internal toString() might change in a future JDK version.
Kevin
@Kevin: Yes, implementation of toString changing in future JDK version sound good enough reason for not doing it, I suppose. :)
Jagmal
@Jagmal: subString is really hurting you. If your listString takes n bytes, then subString temporary adds another n-4 bytes to the heap. By using StringBuilder you will receive your n-4 from "first try"
LicenseQ
A: 

Depending on the need for performance and amount of elements to be added, this might be an ok solution. If the amount of elements are high, the Arraylists reallocation of memory might be a bit slower than StringBuilder.

georg
Can you please explain "the Arraylists reallocation of memory might be a bit slower than StringBuilder."
Jagmal
Arraylist is built on standard arrays and these requires you to set an size of the array(like `int[] a = new int[10];`). This is what the ArrayList is doing for you and as you put in new elements the ArrayList will have to make a larger array and copy all the elements over to the new one.
georg
+1  A: 

ArrayList inherits its toString()-method from AbstractCollection, ie:

public String toString() {
    Iterator<E> i = iterator();
    if (! i.hasNext())
        return "[]";

    StringBuilder sb = new StringBuilder();
    sb.append('[');
    for (;;) {
        E e = i.next();
        sb.append(e == this ? "(this Collection)" : e);
        if (! i.hasNext())
            return sb.append(']').toString();
        sb.append(", ");
    }
}

Building the string yourself will be far more efficient.


If you really want to aggregate the strings beforehand in some sort of List, you should provide your own method to efficiently join them, e.g. like this:

static String join(Collection<?> items, String sep) {
    if(items.size() == 0)
        return "";

    String[] strings = new String[items.size()];
    int length = sep.length() * (items.size() - 1);

    int idx = 0;
    for(Object item : items) {
        String str = item.toString();
        strings[idx++] = str;
        length += str.length();
    }

    char[] chars = new char[length];
    int pos = 0;

    for(String str : strings) {
        str.getChars(0, str.length(), chars, pos);
        pos += str.length();

        if(pos < length) {
            sep.getChars(0, sep.length(), chars, pos);
            pos += sep.length();
        }
    }

    return new String(chars);
}
Christoph
I hope you did benchmarks to confirm that your implementation is better because it not really cleaner than the toString implementation of AbstractCollection. Reader has to really focus on your implementation to understand something while the other is really easy to read.
Matthieu BROUILLARD
A: 

Have you seen this Coding Horror blog entry?

The Sad Tragedy of Micro-Optimization Theater

I am not shure whether or not it is "neater", but from a performance-standpoint it probably won't matter much.

sdfx
+9  A: 

Your approach is dependent on Java's ArrayList#toString() implementation.

While the implementation is documented in the Java API and very unlikely to change, there's a chance it could. It's far more reliable to implement this yourself (loops, StringBuilders, recursion whatever you like better).

Sure this approach may seem "neater" or more "too sweet" or "money" but it is, in my opinion, a worse approach.

Jack Leow
A: 

It seems to me that the StringBuilder will be quick and efficient.

The basic form would look something like this:

public static String concatStrings(List<String> strings)
{
 StringBuilder sb = new StringBuilder();
 for(String s: strings)
 {
  sb.append(s);
 }
 return sb.toString();  
}

If that's too simplistic (and it probably is), you can use a similar approach and add a separator like this:

    public static String concatStringsWSep(List<String> strings, String separator)
{
 StringBuilder sb = new StringBuilder();
 for(int i = 0; i < strings.size(); i++)
 {
  sb.append(strings.get(i));
  if(i < strings.size() - 1)
   sb.append(separator);
 }
 return sb.toString();    
}

I agree with the others who have responded to this question when they say that you should not rely on the toString() method of Java's ArrayList.

codefin
A: 

A variation on codefin's answer

public static String concatStringsWSep(List<String> strings, String separator) {
    StringBuilder sb = new StringBuilder();
    String sep = "";
    for(String s: strings) {
        sb.append(sep).append(s);
        sep = separator;
    }
    return sb.toString();                           
}
Peter Lawrey
Slick! I have never seen the solution in that light. Very nice.
codefin
@codefine See http://stackoverflow.com/questions/58431/algorithm-for-joining-e-g-an-array-of-strings
Jagmal
+9  A: 

Use one of the the StringUtils.join methods in Apache Commons Lang.

import org.apache.commons.lang.StringUtils;

String result = StringUtils.join(list, ", ");
Jeff Olson
A: 

Using the Functional Java library, import these:

import static fj.pre.Monoid.stringMonoid;
import static fj.data.List.list;
import fj.data.List;

... then you can do this:

List<String> ss = list("foo", "bar", "baz");
String s = stringMonoid.join(ss, ", ");

Or, the generic way, if you don't have a list of Strings:

public static <A> String showList(List<A> l, Show<A> s) {
  return stringMonoid.join(l.map(s.showS_()), ", ");
}
Apocalisp
A: 

Wow!!!

How can there be so many variations for doing something as simply concatenating strings sourced from a List ?

Its just a simple loop with a StringBuffer/Builder...

mP
A: 

Assuming it's faster to just move a pointer / set a byte to null (or however Java implements StringBuilder#setLength), rather than check a condition each time through the loop to see when to append the delimiter, you could use this method:

public static String Intersperse (Collection<?> collection, String delimiter)
{
    StringBuilder sb = new StringBuilder ();
    for (Object item : collection)
    {
     if (item == null) continue;
     sb.append (item).append (delimiter);
    }
    sb.setLength (sb.length () - delimiter.length ());
    return sb.toString ();
}