My favorites | Sign in
Project Home Downloads Wiki Issues Source
READ-ONLY: This project has been archived. For more information see this post.
Search
for
  Advanced search   Search tips   Subscriptions
Issue 16: remove use of String Concatenation Operator +
1 person starred this issue and may be notified of changes. Back to list
Status:  Fixed
Owner:  ----
Closed:  May 2007


 
Reported by oliver.lamy@gmail.com, May 8, 2007
Using String Concatenation Operator + can cause some performance issue.
Here a patch which now use StringBuilder instead of String Concatenation
Operator +.
--
Olivier
May 8, 2007
#1 oliver.lamy@gmail.com
Patch attached.
jmesa-16
28.3 KB   Download
May 9, 2007
#2 extremec...@gmail.com
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
#3 oliver.lamy@gmail.com
Oops sorry for junit failure ;-)
I will provide an other patch latter.

May 10, 2007
#4 oliver.lamy@gmail.com
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
#5 oliver.lamy@gmail.com
And for this too : this.prefixId = id + "_";
Not really confident :-)
May 10, 2007
#6 extremec...@gmail.com
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
#7 oliver.lamy@gmail.com
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() + "')");



jmesa-16
16.2 KB   Download
May 10, 2007
#8 oliver.lamy@gmail.com
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.
jmesa-16
16.2 KB   Download
May 13, 2007
#9 extremec...@gmail.com
I am still working on integrating the changes. I'll let you know when I get them done.
May 20, 2007
#10 extremec...@gmail.com
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
#11 extremec...@gmail.com
(No comment was entered for this change.)
Status: Fixed

Powered by Google Project Hosting