Issue 4260 - Replace Apache Commons Lang classes

src/org/adblockplus/android/StringUtils.java

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 package org.adblockplus.android;
 
 public class StringUtils

[Felix Dahlke, 2016/09/12 15:53:28]

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.)

[anton, 2016/09/13 06:27:14]

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.
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.

[Felix Dahlke, 2016/09/13 08:23:29]

Fair enough, let's leave it as is.
I was just pointing out that this is exactly the right approach :) 

[anton, 2016/09/13 10:39:04]

Acknowledged.

 {
-    public static boolean isNotEmpty(String value)
+  public static boolean isNotEmpty(String value)
+  {
+    return value != null && value.length() > 0;
+  }
+
+  public static boolean isEmpty(String value)
+  {
+    return !isNotEmpty(value);
+  }
+
+  public static String join(Object[] array, String separator)
+  {
+    if (array == null)
     {
-        return value != null && value.length() > 0;
+      return null;
     }
 
-    public static boolean isEmpty(String value)
+    StringBuilder sb = new StringBuilder();
+    for (int i = 0; i < array.length; i++)
     {
-        return !isNotEmpty(value);
+      String eachValue = array[i].toString();
+      if (i > 0 && separator != null)
+      {
+        sb.append(separator);
+      }
+
+      if (eachValue != null)

[Felix Dahlke, 2016/09/12 15:53:28]

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.)

[anton, 2016/09/13 06:27:14]

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.

[Felix Dahlke, 2016/09/13 08:23:29]

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.

[anton, 2016/09/13 10:39:04]

Acknowledged.

+      { 
+        sb.append(eachValue);
+      }
     }
 
-    public static String join(Object[] array, String separator)

[anton, 2016/07/22 18:02:17]

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"}, ";"));
	}
}

-    {
-        if (array == null)
-        {
-            return null;
-        }
-
-        StringBuilder sb = new StringBuilder();
-        for (int i = 0; i < array.length; i++)
-        {
-            String eachValue = (String)array[i];

[anton, 2016/07/26 12:45:38]

this could be array[i].toString() but currently works for us

[René Jeschke, 2016/07/26 13:14:44]

Yeah. As we pass in 'Object[]' I really would love to see .toString() here.

[anton, 2016/07/26 13:22:04]

Acknowledged.

-            if (i > 0 && separator != null)
-            {
-                sb.append(separator);
-            }
-
-            if (eachValue != null)
-            { 
-                sb.append(eachValue);
-            }
-        }
-
-        return sb.toString();
-    }
+    return sb.toString();
+  }
 }