You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@pivot.apache.org by Noel Grandin <no...@gmail.com> on 2009/11/17 14:23:47 UTC
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
You're wasting your time and making the code less clear.
See here:
http://c2.com/cgi/wiki?StringBuffer
This is why it is a bad idea to blindly follow the FindBugs report.
smartini@apache.org wrote:
> Author: smartini
> Date: Tue Nov 17 13:19:52 2009
> New Revision: 881271
>
> URL: http://svn.apache.org/viewvc?rev=881271&view=rev
> Log:
> string concatenation improvement (as suggested by FindBugs)
>
> Modified:
> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
>
> Modified: incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
> URL: http://svn.apache.org/viewvc/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java?rev=881271&r1=881270&r2=881271&view=diff
> ==============================================================================
> --- incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java (original)
> +++ incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java Tue Nov 17 13:19:52 2009
> @@ -51,7 +51,7 @@
> }
>
> private void updateSelection(ListView listView) {
> - String selectionText = "";
> + StringBuffer selectionText = new StringBuffer("");
>
> Sequence<Span> selectedRanges = listView.getSelectedRanges();
> for (int i = 0, n = selectedRanges.getLength(); i < n; i++) {
> @@ -61,15 +61,15 @@
> j <= selectedRange.end;
> j++) {
> if (selectionText.length() > 0) {
> - selectionText += ", ";
> + selectionText.append(", ");
> }
>
> String text = (String)listView.getListData().get(j);
> - selectionText += text;
> + selectionText.append(text);
> }
> }
>
> - selectionLabel.setText(selectionText);
> + selectionLabel.setText(selectionText.toString());
> }
> };
>
>
>
>
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Sandro Martini <sa...@gmail.com>.
I don't agree, but ... as requested, Ok.
Only a question, to avoid damages, is it right in Eclipse to revert
using the option "replace from\Revision" ?
The last (before mine) is 810937 of 03/09/09, and the content seems right.
Thanks,
Sandro
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Sandro Martini <sa...@gmail.com>.
Hi, no problem :-) ... but so what have I to do: revert or not revert
(like Shakespeare) ?
But important, to avoid damages, the revert process I wrote before is
it right (I've never done a revert to Subversion in Eclipse, sorry) ?
With the Release approaching I'm doing usual checks with FindBugs ...
so other small things are revealed, and a new mail will follow with
some inside.
Thanks,
Sandro
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Sandro Martini <sa...@gmail.com>.
Ok.
Re: svn commit: r881271 -
/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Greg Brown <gk...@mac.com>.
No, just RSSFeedDemo - I already did ListViews (I just haven't checked
it in yet).
On Nov 17, 2009, at 9:45 AM, Sandro Martini wrote:
> Hi Greg,
> thanks for the infos.
>
> Excuse me, only to see if I have understood well, I have to revert
> both files: ListViews.java and RSSFeedDemo.java, Ok ?
>
> Thanks,
> Sandro
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Sandro Martini <sa...@gmail.com>.
Hi Greg,
thanks for the infos.
Excuse me, only to see if I have understood well, I have to revert
both files: ListViews.java and RSSFeedDemo.java, Ok ?
Thanks,
Sandro
Re: svn commit: r881271 -
/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Greg Brown <gk...@mac.com>.
This page describes how to do it:
http://markphip.blogspot.com/2007/01/how-to-undo-commit-in-subversion.html
I just tested this on ListViews.java, so you can tackle
RSSFeedDemo.java. To bring up the History view, right click on the
file name and select Team > Show History. Then right click on your
revision and select "Revert Changes from Revision nnn". You'll then
need to resubmit.
G
On Nov 17, 2009, at 9:10 AM, Sandro Martini wrote:
> Hi,
> Ok, tell me what to do ... or if someone wants to do the revert, do it
> or (please) tell me the right way to do it :-) .
>
> Thanks,
> Sandro
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Sandro Martini <sa...@gmail.com>.
Hi,
Ok, tell me what to do ... or if someone wants to do the revert, do it
or (please) tell me the right way to do it :-) .
Thanks,
Sandro
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Todd Volkert <tv...@gmail.com>.
However, I'd throw into the ring that StringBuilder is a better choice than
StringBuffer for single-threaded situations because StringBuffer is
unnecessarily (in this case) thread-safe.
-T
On Tue, Nov 17, 2009 at 8:46 AM, Noel Grandin <no...@gmail.com> wrote:
>
> My apologies Sandro, Greg is correct, the compiler does not optimise
> this case properly (checked with JDK1.6.0_14)
>
> -- Noel.
>
> Greg Brown wrote:
> > I believe the compiler is capable of optimizing this:
> >
> > String s = "a" + "b" + "c";
> >
> > but not necessarily this:
> >
> > String s = "a";
> > s += "b";
> > s += "c";
> >
> > So FindBugs may not have been wrong to suggest the change. However,
> > one case was a test app and the other was a tutorial (where clarity is
> > much more important than performance), so please revert this change.
> > Thanks.
> >
> > G
> >
> > On Nov 17, 2009, at 8:23 AM, Noel Grandin wrote:
> >
> >>
> >> You're wasting your time and making the code less clear.
> >>
> >> See here:
> >> http://c2.com/cgi/wiki?StringBuffer
> >>
> >> This is why it is a bad idea to blindly follow the FindBugs report.
> >>
> >>
> >> smartini@apache.org wrote:
> >>> Author: smartini
> >>> Date: Tue Nov 17 13:19:52 2009
> >>> New Revision: 881271
> >>>
> >>> URL: http://svn.apache.org/viewvc?rev=881271&view=rev
> >>> Log:
> >>> string concatenation improvement (as suggested by FindBugs)
> >>>
> >>> Modified:
> >>>
> >>>
> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
> >>>
> >>>
> >>> Modified:
> >>>
> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
> >>>
> >>> URL:
> >>>
> http://svn.apache.org/viewvc/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java?rev=881271&r1=881270&r2=881271&view=diff
> >>>
> >>>
> ==============================================================================
> >>>
> >>> ---
> >>>
> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
> >>> (original)
> >>> +++
> >>>
> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
> >>> Tue Nov 17 13:19:52 2009
> >>> @@ -51,7 +51,7 @@
> >>> }
> >>>
> >>> private void updateSelection(ListView listView) {
> >>> - String selectionText = "";
> >>> + StringBuffer selectionText = new StringBuffer("");
> >>>
> >>> Sequence<Span> selectedRanges =
> >>> listView.getSelectedRanges();
> >>> for (int i = 0, n = selectedRanges.getLength(); i < n;
> >>> i++) {
> >>> @@ -61,15 +61,15 @@
> >>> j <= selectedRange.end;
> >>> j++) {
> >>> if (selectionText.length() > 0) {
> >>> - selectionText += ", ";
> >>> + selectionText.append(", ");
> >>> }
> >>>
> >>> String text =
> >>> (String)listView.getListData().get(j);
> >>> - selectionText += text;
> >>> + selectionText.append(text);
> >>> }
> >>> }
> >>>
> >>> - selectionLabel.setText(selectionText);
> >>> + selectionLabel.setText(selectionText.toString());
> >>> }
> >>> };
> >>>
> >>>
> >>>
> >>>
> >>
> >
>
>
Re: svn commit: r881271 - /incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Noel Grandin <no...@gmail.com>.
My apologies Sandro, Greg is correct, the compiler does not optimise
this case properly (checked with JDK1.6.0_14)
-- Noel.
Greg Brown wrote:
> I believe the compiler is capable of optimizing this:
>
> String s = "a" + "b" + "c";
>
> but not necessarily this:
>
> String s = "a";
> s += "b";
> s += "c";
>
> So FindBugs may not have been wrong to suggest the change. However,
> one case was a test app and the other was a tutorial (where clarity is
> much more important than performance), so please revert this change.
> Thanks.
>
> G
>
> On Nov 17, 2009, at 8:23 AM, Noel Grandin wrote:
>
>>
>> You're wasting your time and making the code less clear.
>>
>> See here:
>> http://c2.com/cgi/wiki?StringBuffer
>>
>> This is why it is a bad idea to blindly follow the FindBugs report.
>>
>>
>> smartini@apache.org wrote:
>>> Author: smartini
>>> Date: Tue Nov 17 13:19:52 2009
>>> New Revision: 881271
>>>
>>> URL: http://svn.apache.org/viewvc?rev=881271&view=rev
>>> Log:
>>> string concatenation improvement (as suggested by FindBugs)
>>>
>>> Modified:
>>>
>>> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
>>>
>>>
>>> Modified:
>>> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
>>>
>>> URL:
>>> http://svn.apache.org/viewvc/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java?rev=881271&r1=881270&r2=881271&view=diff
>>>
>>> ==============================================================================
>>>
>>> ---
>>> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
>>> (original)
>>> +++
>>> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
>>> Tue Nov 17 13:19:52 2009
>>> @@ -51,7 +51,7 @@
>>> }
>>>
>>> private void updateSelection(ListView listView) {
>>> - String selectionText = "";
>>> + StringBuffer selectionText = new StringBuffer("");
>>>
>>> Sequence<Span> selectedRanges =
>>> listView.getSelectedRanges();
>>> for (int i = 0, n = selectedRanges.getLength(); i < n;
>>> i++) {
>>> @@ -61,15 +61,15 @@
>>> j <= selectedRange.end;
>>> j++) {
>>> if (selectionText.length() > 0) {
>>> - selectionText += ", ";
>>> + selectionText.append(", ");
>>> }
>>>
>>> String text =
>>> (String)listView.getListData().get(j);
>>> - selectionText += text;
>>> + selectionText.append(text);
>>> }
>>> }
>>>
>>> - selectionLabel.setText(selectionText);
>>> + selectionLabel.setText(selectionText.toString());
>>> }
>>> };
>>>
>>>
>>>
>>>
>>
>
Re: svn commit: r881271 -
/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java
Posted by Greg Brown <gk...@mac.com>.
I believe the compiler is capable of optimizing this:
String s = "a" + "b" + "c";
but not necessarily this:
String s = "a";
s += "b";
s += "c";
So FindBugs may not have been wrong to suggest the change. However,
one case was a test app and the other was a tutorial (where clarity is
much more important than performance), so please revert this change.
Thanks.
G
On Nov 17, 2009, at 8:23 AM, Noel Grandin wrote:
>
> You're wasting your time and making the code less clear.
>
> See here:
> http://c2.com/cgi/wiki?StringBuffer
>
> This is why it is a bad idea to blindly follow the FindBugs report.
>
>
> smartini@apache.org wrote:
>> Author: smartini
>> Date: Tue Nov 17 13:19:52 2009
>> New Revision: 881271
>>
>> URL: http://svn.apache.org/viewvc?rev=881271&view=rev
>> Log:
>> string concatenation improvement (as suggested by FindBugs)
>>
>> Modified:
>> incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/
>> lists/ListViews.java
>>
>> Modified: incubator/pivot/trunk/tutorials/src/org/apache/pivot/
>> tutorials/lists/ListViews.java
>> URL: http://svn.apache.org/viewvc/incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/lists/ListViews.java?rev=881271&r1=881270&r2=881271&view=diff
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =
>> =====================================================================
>> --- incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/
>> lists/ListViews.java (original)
>> +++ incubator/pivot/trunk/tutorials/src/org/apache/pivot/tutorials/
>> lists/ListViews.java Tue Nov 17 13:19:52 2009
>> @@ -51,7 +51,7 @@
>> }
>>
>> private void updateSelection(ListView listView) {
>> - String selectionText = "";
>> + StringBuffer selectionText = new StringBuffer("");
>>
>> Sequence<Span> selectedRanges =
>> listView.getSelectedRanges();
>> for (int i = 0, n = selectedRanges.getLength(); i < n; i
>> ++) {
>> @@ -61,15 +61,15 @@
>> j <= selectedRange.end;
>> j++) {
>> if (selectionText.length() > 0) {
>> - selectionText += ", ";
>> + selectionText.append(", ");
>> }
>>
>> String text = (String)listView.getListData().get
>> (j);
>> - selectionText += text;
>> + selectionText.append(text);
>> }
>> }
>>
>> - selectionLabel.setText(selectionText);
>> + selectionLabel.setText(selectionText.toString());
>> }
>> };
>>
>>
>>
>>
>