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. |
DescriptionIssue 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
MessagesTotal messages: 11
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:32: public static String join(Object[] array, String separator) tested according to https://commons.apache.org/proper/commons-lang/apidocs/org/apache/commons/lan... docs: package org.adblockplus.libadblockplus.tests; import org.adblockplus.android.StringUtils; import org.junit.Test; import junit.framework.*; public class StringUtilsTest extends TestCase { @Test public void testEmpty() { assertTrue(StringUtils.isEmpty(null)); assertTrue(StringUtils.isEmpty("")); assertFalse(StringUtils.isEmpty(" ")); assertFalse(StringUtils.isEmpty("bob")); assertFalse(StringUtils.isEmpty(" bob ")); } @Test public void testJoin() { assertNull(StringUtils.join(null, "")); assertEquals("", StringUtils.join(new Object[] {}, "")); assertEquals("", StringUtils.join(new Object[] {null}, "")); assertEquals("a;b;c", StringUtils.join(new Object[] {"a", "b", "c"}, ";")); assertEquals("abc", StringUtils.join(new Object[] {"a", "b", "c"}, null)); assertEquals(";;a", StringUtils.join(new Object[] {null, "", "a"}, ";")); } }
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:42: String eachValue = (String)array[i]; this could be array[i].toString() but currently works for us
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... 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... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:42: String eachValue = (String)array[i]; On 2016/07/26 12:45:38, anton wrote: > this could be array[i].toString() but currently works for us Yeah. As we pass in 'Object[]' I really would love to see .toString() here.
https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... File src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... src/org/adblockplus/android/AndroidUpdateCheckDoneCallback.java:21: import org.adblockplus.android.StringUtils;; On 2016/07/26 13:14:44, René Jeschke wrote: > Double colon here :-) Acknowledged. https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348633/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:42: String eachValue = (String)array[i]; On 2016/07/26 13:14:44, René Jeschke wrote: > On 2016/07/26 12:45:38, anton wrote: > > this could be array[i].toString() but currently works for us > > Yeah. As we pass in 'Object[]' I really would love to see .toString() here. Acknowledged.
https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:22: public static boolean isNotEmpty(String value) We're using two-space indentation. ;-)
https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... File src/org/adblockplus/android/StringUtils.java (right): https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:22: public static boolean isNotEmpty(String value) On 2016/07/28 11:46:22, René Jeschke wrote: > We're using two-space indentation. ;-) Yes, i know. I've been thinking we can approve these changes, then approve codestyle issue and reformat all the code according to approved rules. Do you want me to reformat all the pending codereviews including this one now?
On 2016/07/28 11:49:00, anton wrote: > https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... > File src/org/adblockplus/android/StringUtils.java (right): > > https://codereview.adblockplus.org/29348632/diff/29348677/src/org/adblockplus... > src/org/adblockplus/android/StringUtils.java:22: public static boolean > isNotEmpty(String value) > On 2016/07/28 11:46:22, René Jeschke wrote: > > We're using two-space indentation. ;-) > > Yes, i know. I've been thinking we can approve these changes, then approve > codestyle issue and reformat all the code according to approved rules. Do you > want me to reformat all the pending codereviews including this one now? LGTM Patch Set 2 + 3
Looks good, just a comment about a comment and a technical question. (As Diego hinted, patch set 3 is missing the previous changes - every patch set should include the entire change.) 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 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. (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.) https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:48: if (eachValue != null) 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.)
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/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. > > (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. https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:48: if (eachValue != null) 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.
LGTM 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 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 :) https://codereview.adblockplus.org/29348632/diff/29348915/src/org/adblockplus... src/org/adblockplus/android/StringUtils.java:48: if (eachValue != null) 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.
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. 
|
