Community
Participate
Working Groups
Convert setFileName("ComponentWiring" + nameSuffix + "." + CWEditor.MODEL_FILE_EXTENSION); //$NON-NLS-2$//$NON-NLS-1$ To StringBuffer buf = new StringBuffer(); buf.append("ComponentWiring"); //$NON-NLS-1$ buf.append(nameSuffix); buf.append("."); //$NON-NLS-1$ buf.append(CWEditor.MODEL_FILE_EXTENSION); setFileName(buf.toString());
Good suggestion. Should consider for 2.2
Most compilers (javac, eclipse) do this optimization when generating byte code.
The suggestion to convert to string buffer is also useful in loops: String data = ""; for(int i=0;i<args.length;i++) { data += args[i]; } I don't believe these kind of loops will be picked up by the compiler, but I could be wrong.
This is now more a quick assist than a refactoring.
Currently no action planned for 3.1
*** Bug 163960 has been marked as a duplicate of this bug. ***
reopen
*** Bug 154199 has been marked as a duplicate of this bug. ***
*** Bug 48240 has been marked as a duplicate of this bug. ***
Created attachment 81312 [details] Convert to StringBuffer quickfix Hi, attached is a patch for a new quickfix to convert concatenated strings into a string buffer. As this is the first time for me hacking JDT it would be really great to have some feedback how to improve the implementation. Thanks in advance!
Hi Benjamin, looks good! From a quick look at the code, I saw the following: - resolveBinding can be null, if there are compile errors - a statement can be in a if body (if (x) statement();). Test this with: ASTNodes.isControlStatementBody(statement.getLocationInParent()) In that case you have to introduce a block. - For variable name suggestions use: StubUtility.getVariableNameSuggestions. - Use ASTRewrite.createCopyPlaceholder when copying nodes. You shouldn't be copying trees here. If it is a tree, wouldn't it be necessary to traverse all children? - Add a test case for the following scenario: foo("Hello" + "World")
Created attachment 81817 [details] updated patch (In reply to comment #11) > Hi Benjamin, > looks good! From a quick look at the code, I saw the following: First, thanks for the good hints :) > - resolveBinding can be null, if there are compile errors Done > - a statement can be in a if body (if (x) statement();). Test this with: > ASTNodes.isControlStatementBody(statement.getLocationInParent()) > In that case you have to introduce a block. Done - after a bit struggeling ;) > - For variable name suggestions use: StubUtility.getVariableNameSuggestions. I thought that there has to be something like that but could not find it :) > - Use ASTRewrite.createCopyPlaceholder when copying nodes. You shouldn't be > copying trees here. If it is a tree, wouldn't it be necessary to traverse all > children? Done > - Add a test case for the following scenario: foo("Hello" + "World") Done I also added a testcase for NLS-comment handling but I think this could be done as an additional step. Martin, what do you think of Alex comment #3 - should be also support this case? I think this you get a little tricky (at least for me :) ).
Created attachment 82032 [details] updated patch Just saw that i forgot the NLS test (I commented it out as this is not yet implemented - waiting for response of Martin). Some thoughts about the two missing features: * NLS support: I could use NLSUtil.createNLSEdit to do the work but it will only give me a TextEdit which I am not able to apply to the resulting AST (afaik). One idea would be to implement a CompositeProposal which manages two proposals as one (the ASTRewritePropsal used for the rewrite itself and one to add the TextEdit computed by the NLS stuff. Or should the whole thing be done by duplicating the NLS stuff and have an AST-conform way? Or is there maybe already something like this somewhere? ;) * Loops: I think this could be done and would be a good thing but it could get a bit complex as far as I can see. I'm willing to contribute this too in an additional step when the initial support is accepted. Have a nice weekend!
I'll have a look as I soon as I find time for it. Thanks Benjamin. For NLS: None of our quick fixes or refactoring tries to preserve the NON_NLS comments. So no need for that here.
I simplified the patch a bit, in particular the evaluation how the InfixExpression is found. Released the code, but not the tests> 20070811. Thanks Benjamin! Can you have a look at the following outstanding issues: For the tests you used ConvertIterableLoopQuickFixTest as example. Unfortunately this is a bad one, as I just realized now. Can you have a look at the other quick fix tests, for example AssistQuickFixTest - use spaces, no tabs in example code. Make sure that the example results are in a good format. It doesn't make sense to test for badly indented code... :-) - Use 'assertExpectedExistInProposals' to test that the StringBuffer proposal is in the list. - I wouldn't mind to include the StringBuffer tests in AssistQuickFixTest.
Created attachment 82822 [details] updated tests Martin, here are the updated test cases. Any other things to do?
tests released (removed some IMO redundant tests) > 20071115 Thanks Benjamin! I think we're done with everthibg. If you're interested in adding some improvement, you're welcome! Interesting could be: a. 'Convert to MessageFormat' (bug 29562) String s= "Hello" + name + "!"; -> MessageFormat.format("Hello {0}!", new Object[] { name }); b. look at multiple assignments String s= "Hello" s+= " World"; s= s + "!"; Please open a new bug if you're interested in tackle this.
How about support for transforming constructs like "Foo: " + foo + " and bar is " + bar into String.format("Foo: %s and bar is %s",foo,bar)
Is this use case which was in Bug 48240 covered ? <pre> ... String s = ""; for(int i = 0; i < MAX; i++) { switch (i % 3) { case 0 : s += "a"; break; case 1 : s += "b"; break; case 2 : default : s = "c" + s; } } System.out.println(s); ... </pre> and when you select "s" and choose the quick assist "convert to StringBuffer" or "convert to StringBuilder" (for 1.5) you'll have <pre> ... StringBuffer s = new StringBuffer(""); for(int i = 0; i < MAX; i++) { switch (i % 3) { case 0 : s.append("a"); break; case 1 : s.append("b"); break; case 2 : default : s.insert(0, "c"); } } System.out.println(s); ... </pre>
Don't need to open a new bug for MessageFormat, it already exists here Bug 29562 May be it should be extended to handle Java 5 java.util.Formatter and by the way java.lang.String.format()
Do you think it is worth adding a getConvertToStringBuilderProposal based on getConvertToStringBufferProposal which could be available for 1.5 and upper ?
Maybe it would make sense to automatically StringBuilder when in 1.5. The quick assist could set up a the linked mode with proposals where it is possible to choose between StringBuilder and StringBuffer. I'm fine with adding 1 or two more variants, but I think we should be careful to keep the number of proposals small.
IMHO StringBuilder should be used if the lib&class compatibility are 5.0, especially because the compiler internally converts similar concatenations to StringBuilder.
Re StringBuilder: Either reopen this bug or delete the duplicate mark on bug 154199, to allow to track the "StringBuilder" code assist.
