Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29348632: Issue 4260 - Replace Apache Commons Lang classes (Closed)

Created:
July 22, 2016, 5:58 p.m. by anton
Modified:
April 17, 2017, 7:08 a.m.
CC:
René Jeschke
Visibility:
Public.

Description

Issue 4260 - Replace Apache Commons Lang classes #depends on https://codereview.adblockplus.org/29345540/

Patch Set 1 #

Total comments: 6

Patch Set 2 : full changeset including rene's comments #

Total comments: 2

Patch Set 3 : two-space indentation #

Total comments: 8
Unified diffs Side-by-side diffs Delta from patch set Stats (+27 lines, -27 lines) Patch
M src/org/adblockplus/android/StringUtils.java View 1 2 1 chunk +27 lines, -27 lines 8 comments Download

Messages

Total messages: 11
anton
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/StringUtils.java#newcode32 src/org/adblockplus/android/StringUtils.java:32: public static String join(Object[] array, String separator) tested according ...
July 22, 2016, 6:02 p.m. (2016-07-22 18:02:18 UTC) #1
anton
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/StringUtils.java#newcode42 src/org/adblockplus/android/StringUtils.java:42: String eachValue = (String)array[i]; this could be array[i].toString() but ...
July 26, 2016, 12:45 p.m. (2016-07-26 12:45:38 UTC) #2
René Jeschke
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java#newcode21 src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:21: import org.adblockplus.android.StringUtils;; Double colon here :-) https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java ...
July 26, 2016, 1:14 p.m. (2016-07-26 13:14:44 UTC) #3
anton
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java#newcode21 src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:21: import org.adblockplus.android.StringUtils;; On 2016/07/26 13:14:44, René Jeschke wrote: > ...
July 26, 2016, 1:22 p.m. (2016-07-26 13:22:05 UTC) #4
René Jeschke
https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java#newcode22 src/org/adblockplus/android/StringUtils.java:22: public static boolean isNotEmpty(String value) We're using two-space indentation. ...
July 28, 2016, 11:46 a.m. (2016-07-28 11:46:23 UTC) #5
anton
https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java#newcode22 src/org/adblockplus/android/StringUtils.java:22: public static boolean isNotEmpty(String value) On 2016/07/28 11:46:22, René ...
July 28, 2016, 11:49 a.m. (2016-07-28 11:49:00 UTC) #6
diegocarloslima
On 2016/07/28 11:49:00, anton wrote: > https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java > File src/org/adblockplus/android/StringUtils.java (right): > > https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus/android/StringUtils.java#newcode22 > ...
Sept. 8, 2016, 2:42 p.m. (2016-09-08 14:42:45 UTC) #7
Felix Dahlke
Looks good, just a comment about a comment and a technical question. (As Diego hinted, ...
Sept. 12, 2016, 3:53 p.m. (2016-09-12 15:53:29 UTC) #8
anton
https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus/android/StringUtils.java#newcode20 src/org/adblockplus/android/StringUtils.java:20: public class StringUtils On 2016/09/12 15:53:28, Felix Dahlke wrote: ...
Sept. 13, 2016, 6:27 a.m. (2016-09-13 06:27:15 UTC) #9
Felix Dahlke
LGTM https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus/android/StringUtils.java File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus/android/StringUtils.java#newcode20 src/org/adblockplus/android/StringUtils.java:20: public class StringUtils On 2016/09/13 06:27:14, anton wrote: ...
Sept. 13, 2016, 8:23 a.m. (2016-09-13 08:23:30 UTC) #10
anton
Sept. 13, 2016, 10:39 a.m. (2016-09-13 10:39:04 UTC) #11
https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus...
File src/org/adblockplus/android/StringUtils.java (right):

https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus...
src/org/adblockplus/android/StringUtils.java:20: public class StringUtils
On 2016/09/13 08:23:29, Felix Dahlke wrote:
> On 2016/09/13 06:27:14, anton wrote:
> > On 2016/09/12 15:53:28, Felix Dahlke wrote:
> > > Might be worth a doc comment that this is supposed to be a drop-in
> replacement
> > > for Apache Commons' StringUtils? Wouldn't insist on it though.
> > 
> > Actually this class has exactly the same methods signatures as original
Apache
> > Commons in order not to change anything except package. But it's not
> > general-purpose replacement and it will not be used outside of the project
> (most
> > likely), so i don't think we need javadoc here. Also methods names are
pretty
> > self-described.. Let me know if i should it anyway.
> 
> Fair enough, let's leave it as is.
> 
> > > 
> > > (On another note: I double checked that our code actually differs from
> Aparche
> > > Commons' StringUtils, which it does. if it wouldn't, it would be alright
as
> > > well, but we should then point out in a comment that we are using some
code
> > from
> > > that project here.)
> > 
> > We were using Apache Commons and i'm not sure we've added that we are using
> it.
> > However this task is for replacing of apache commons classes so no Apache
code
> > is used. For new methods impls we're not using any Apache code - i've took
the
> > tests in original apache commons javadocs and wrote the code to pass the
> tests.
> 
> I was just pointing out that this is exactly the right approach :) 

Acknowledged.

https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus...
src/org/adblockplus/android/StringUtils.java:48: if (eachValue != null)
On 2016/09/13 08:23:29, Felix Dahlke wrote:
> On 2016/09/13 06:27:14, anton wrote:
> > On 2016/09/12 15:53:28, Felix Dahlke wrote:
> > > So if the value is null, we'd just print the separator? Is that the
intended
> > > behaviour? (I suppose a few tests wouldn't hurt.)
> > 
> > There is a difference between null string and empty ("") string.
> > 
> > For null i believe it's occasionally added and i believe we should just
avoid
> > it. F.e. for 3 items array {"1",null,"2"} we should think it's only 2 items
> > {"1","2"} and only 1 separator should be added.
> > 
> > But empty string is full-featured string and we can't avoid it and 2
> separators
> > should be added for {"1", "", "3"}.
> > 
> > To be discussed though.
> 
> Makes sense, just wanted to make sure that's the intended behaviour. I looked
at
> the Apache Commons code for this and it does behave the same way, so that's
> fine.

Acknowledged.

Powered by Google App Engine
This is Rietveld