views:

314

answers:

8

I have code as follows :

String s = "";
for (My my : myList) {
    s += my.getX();
}

Findbugs always reports error when I do this.

+5  A: 

The String object is immutable in Java. Each + means another object. You could use StringBuffer to minimize the amount of created objects.

John B.
Java translates concatenation with + to use a `StringBuilder`. You only need to use the `StringBuilder` yourself if you're looping or building a string using a series of method calls or some such. Also, `StringBuffer` has thread safety overhead that you usually don't need, so `StringBuilder` is generally a better choice.
ColinD
_Tony scurries away to read about StringBuilder_
Tony Ennis
+16  A: 

I would use + if you are manually concatenating,

String word = "Hello";
word += " World!";

However, if you are iterating and concatenating I would suggest StringBuilder,

StringBuilder sb = new StringBuilder();
for (My my : myList) {
    sb.append(my.getX());
}
Anthony Forloney
+1 - it is concatenation *in a loop* that you should generally avoid.
Stephen C
Another point: it can actually be _better_ to use + in some cases, if there are string literals being concatenated, since a series of concatenated literals will be compiled as one string. I'm not positive, given the way it's split on multiple lines, but I think the compiler would optimize that to `String word = "Hello World!"`.
ColinD
It is OK to use concatenation like this, if you are sure that never, ever your application will be localized. If you are planning to translate it to foreign language, you will end up with localizability defect.
Paweł Dyda
However '+' and '+=' are not equivalent in efficiency; e.g. `word = a + b + c;` will probably be significantly faster than `word = a; word += b; word += c;`.
Stephen C
Because a + b + c uses a StringBuffer behind the scenes.
Seun Osewa
+3  A: 

The compiler can optimize some thing such as

"foo"+"bar"

To

StringBuilder s1=new StringBuilder(); s1.append("foo").append("bar");

However this is still suboptimal since it starts with a default size of 16. As with many things though you should find your biggest bottle necks and work your way down the list. It doesn't hurt to be in the habbit of using a SB pattern from the get go though, especially if you're able to calculate an optimal initialization size.

monitorme
actually the *compiler* can optimize this to just `"foobar"`... +1 for finding the bottleneck
Carlos Heuberger
+3  A: 

Premature optimization can be bad as well as it often reduces readability and is usually completely unnecessary. Use + if it is more readable unless you actually have an overriding concern.

Peter DeWeese
+3  A: 

It is not 'always bad' to use "+". Using StringBuffer everywhere can make code really bulky.

If someone put a lot of "+" in the middle of an intensive, time-critical loop, I'd be annoyed. If someone put a lot of "+" in a rarely-used piece of code I would not care.

Tony Ennis
+1  A: 

I would say use plus in the following:

String c = "a" + "b"

And use StringBuilder class everywhere else. As already mentioned in the first case it will be optimized by the compiler and it's more readable.

negative
+3  A: 

Each time you do string+=string, it calls method like this:

private String(String s1, String s2) {
    if (s1 == null) {
        s1 = "null";
    }
    if (s2 == null) {
        s2 = "null";
    }
    count = s1.count + s2.count;
    value = new char[count];
    offset = 0;
    System.arraycopy(s1.value, s1.offset, value, 0, s1.count);
    System.arraycopy(s2.value, s2.offset, value, s1.count, s2.count);
}

In case of StringBuilder, it comes to:

final void append0(String string) {
    if (string == null) {
        appendNull();
        return;
    }
    int adding = string.length();
    int newSize = count + adding;
    if (newSize > value.length) {
        enlargeBuffer(newSize);
    }
    string.getChars(0, adding, value, count);
    count = newSize;
}

As you can clearly conclude, string + string creates a lot of overhead, and in my opinion should be avoided if possible. If you think using StringBuilder is bulky or to long you can just make a method and use it indirectly, like:

public static String scat(String... vargs) {
    StringBuilder sb = new StringBuilder();

    for (String str : vargs)
        sb.append(str);

    return sb.toString();
}

And use it like:

String abcd = scat("a","b","c","d"); 

In C# I'm told its about as same as string.Concat();. In your case it would be wise to write overload for scat, like:

public static String scat(Collection<?> vargs) {
    StringBuilder sb = new StringBuilder();

    for (Object str : vargs)
        sb.append(str);

    return sb.toString();
}

Then you can call it with:

result = scat(myList)
Margus
+1  A: 

One of the reasons why FindBugs should argue about using concatenation operator (be it "+" or "+=") is localizability. In the example you gave it is not so apparent, but in case of the following code it is:

String result = "Scanning found " + Integer.toString(numberOfViruses) + " viruses";

If this looks somewhat familiar, you need to change your coding style. The problem is, it will sound great in English, but it could be a nightmare for translators. That's just because you cannot guarantee that order of the sentence will still be the same after translation – some languages will be translated to "1 blah blah", some to "blah blah 3". In such cases you should always use MessageFormat.format() to build compound sentences and using concatenation operator is clearly internationalization bug.

BTW. I put another i18n defect here, could you spot it?

Paweł Dyda