| Issue 16: | remove use of String Concatenation Operator + | |
| 1 person starred this issue and may be notified of changes. | Back to list |
Using String Concatenation Operator + can cause some performance issue. Here a patch which now use StringBuilder instead of String Concatenation Operator +. -- Olivier
May 9, 2007
First of off I really appreciate the patches! I agree with most of the changes. I see where I intermingled StringBuilder and String concatenation together, and that was a good catch. Wherever StringBuilder and String concatenation exist it should just be using the StringBuilder. A StringBuilder should also be used instead of the += syntax. I would have to disagree with changing the inline Strings though. On one hand you just cannot go wrong with using a StringBuilder. Thats a given. However, if you take in account that the compiler will do some optimizations for you the need to always use a StringBuilder goes down quite a bit. From what I understand if you have Strings that are concatenated together on one line and not doing the += operation then the compiler will automatically build a StringBuffer() at compile time anyway. So for small inline Strings it is not worth making the code slightly less readable when the performance gains are not there or do not exist. I went searching for a document that spelled it out and I found this on Sun's site: http://java.sun.com/developer/JDCTechTips/2002/tt0305.html It is a little old, but if anything the new JVM's should be even smarter now. With this is mind it would be great if the patch were to follow these guidelines. Small inline String concatenation is fine but mixing builders and strings or using the += operation should be avoided. Also, when submitting the patches make sure the junit tests still pass. With the current patch some of the unit tests fail. You can run them at the top level test directory right out of eclipse. If you still have concerns we can talk more.
May 10, 2007
Oops sorry for junit failure ;-) I will provide an other patch latter.
May 10, 2007
Do you mean the following code doens't need to StringBuilder or StringBuffer : StringUtils.substringAfter(parameter, prefixId + Action.SORT.toParam() + position + "_"); And we must confident to the compiler to replace this with a StringBuffer use ?
May 10, 2007
And for this too : this.prefixId = id + "_"; Not really confident :-)
May 10, 2007
You should do a test then to prove that it works that way. Similar to the way that the article performed the test. Java 6 ships with JConsole so you should see a huge spike in memory and object creation if the compiler is not doing what I would expect. The += logic seems to be the real evil logic. We are trusting the compiler to do the right thing, but I would consider this a feature of the compiler. Its not worth reducing the readability of the code to solve phantom problems. The places where the StringBuilder and String concentation is mixed up is just poor practice on my part so I like fixing that logic.
May 10, 2007
Here a patch.
No junit failed :-).
I have fixed junits with maven2, now with the cli : mvn clean test works.
I prefer this than junit with eclipse :-)
I don't have replace all + string concatenation.
I will try add other impls to test it.
I'am really confident with some like :
- StringBuilder action = new StringBuilder("setMaxRowsToLimit('" + limit.getId()
+ "', this.options[this.selectedIndex].value);onInvokeAction('" + limit.getId() + "')");
or
- StringBuilder action = new StringBuilder("javascript:");
action.append("setPageToLimit('" + limit.getId() + "','" + 1 + "');" +
getOnInvokeAction() + "('" + limit.getId() + "')");
or
- html.onclick("addSortToLimit('" + limit.getId() + "','" + position + "','" +
column.getProperty() + "','" + Order.ASC.toParam() + "');onInvokeAction('" +
limit.getId() + "')");
May 10, 2007
Here a patch. No junit no more failed :-). I have fixed junits with maven2, now with the cli : mvn clean test works. I prefer this than junit with eclipse :-) I don't have replace all + string concatenation. I will try add other impls to test it.
May 13, 2007
I am still working on integrating the changes. I'll let you know when I get them done.
May 20, 2007
I finally found the time to integrate the changes. As we talked about I did not take the changes that merely took inline Strings and made them StringBuilders. I still think the best approach would be to use a profiler to see if it is apparent that the compiler is not automatically doing the right thing. If we knew that then it would be easy to accept the changes. The changes I would take though is where I mixed StringBuilder and String concatenation code.
Status:
Started
May 22, 2007
(No comment was entered for this change.)
Status:
Fixed
|
28.3 KB Download