3
\$\begingroup\$

I have this code, which goes through a string and split it using a char delimiter.

I am concerned where it can be improved.

  • Main concerns are speed and memory usage with speed as the priority.
  • Any other improvements are welcome too.
  • Is there a better way (Without regex based split and without 3rd party libraries).

What it does:

  • Iterate through string and extract all portions of strings delimited by given char
  • return all portions, including empty ones.
  • StringUtility class name is used because in future I can add other utility methods when necessory

Code:

public class StringUtility {

    /**
     * Split a string using a single char delimiter
     * @param strToSplit string to use
     * @param delimiter delimiter
     * @return String[] of portions
     */
    public static String[] split(String strToSplit, char delimiter) {
        ArrayList<String> arr = new ArrayList<String>();
        StringBuilder sb = new StringBuilder();
        for (int i = 0; i < strToSplit.length(); i++) {
            char at = strToSplit.charAt(i);
            if (at == delimiter) {
                arr.add(sb.toString());
                sb = new StringBuilder();
            } else {
                sb.append(at);
            }
        }
        arr.add(sb.toString());
        return arr.toArray(new String[0]);
    }
}

Unit Test:

This code currently passes following jUnit Test Cases

public class StringUtilityTest {

    @Test
    public void test1() {
        final String str1 = "Because";
        final String str2 = "I'm";
        final String str3 = "Batman";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3,
                delim);
        Assert.assertTrue(parts.length == 3);
        Assert.assertTrue(parts[0].equals(str1));
        Assert.assertTrue(parts[1].equals(str2));
        Assert.assertTrue(parts[2].equals(str3));
    }

    @Test
    public void test2() {
        final String str1 = "";
        final String str2 = "I'm";
        final String str3 = "Batman";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3,
                delim);
        Assert.assertTrue(parts.length == 3);
        Assert.assertTrue(parts[0].equals(str1));
        Assert.assertTrue(parts[1].equals(str2));
        Assert.assertTrue(parts[2].equals(str3));
    }

    @Test
    public void test3() {
        final String str1 = "";
        final char delim = ' ';
        String[] parts = StringUtility.split(str1,
                delim);
        Assert.assertTrue(parts.length == 1);
        Assert.assertTrue(parts[0].equals(str1));
    }
}

Speed Test:

From fastest to slowest

  1. janos/Heslacher : split using index of and subString
  2. Emanuele Paolini : split using subString
  3. janos : split by setting string builder to zero length
  4. me : original
\$\endgroup\$
4
  • 2
    \$\begingroup\$ Just to be sure, you are looking to reinvent the wheel, right? Because String#split already exists. \$\endgroup\$
    – skiwi
    Aug 5, 2014 at 15:45
  • \$\begingroup\$ Java 5 gave us the enhanced for loop. And Java 8 gives us streams - why are you programming in Java 4? \$\endgroup\$ Aug 6, 2014 at 12:56
  • \$\begingroup\$ @MartinSchröder: Thanks for the comment, I am puzzled and I cannot yet think of a way to use enhanced for loops or streams to split a string (question scope: using a char without using regex or 3rd party libraries). Any version of java is fair game. You are welcome to post an answer that use enhanced for loops or streams. It will be a nice addition. \$\endgroup\$ Aug 6, 2014 at 16:42
  • 1
    \$\begingroup\$ @skiwi I've added the reinvent the wheel tag \$\endgroup\$ Aug 24, 2014 at 12:00

3 Answers 3

6
\$\begingroup\$

Sloppiness

There are several careless mistakes that are easy to fix.

Don't use implementation types in declaration, unless you really need to use a specific feature of that implementation:

ArrayList<String> arr = new ArrayList<String>();

Use the interface type instead:

List<String> arr = new ArrayList<String>();

Instead of creating a new StringBuilder, it might be faster to reset it, like this:

sb.setLength(0);

See this discussion for more details.


Don't convert a List to an array like this:

return arr.toArray(new String[0]);

It's recommended to pass a correctly sized array:

return arr.toArray(new String[arr.size()]);

Better unit tests

Give your test cases meaningful names. So that when your IDE reports some of them failing, it will be easier to understand what failed. For example:

@Test
public void testNormalSentence() {
    final String str1 = "Because";
    final String str2 = "I'm";
    final String str3 = "Batman";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
    Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));
}

@Test
public void testStartingWithDelim() {
    final String str1 = "";
    final String str2 = "I'm";
    final String str3 = "Batman";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
    Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));
}

@Test
public void testEmptyString() {
    final String str1 = "";
    final char delim = ' ';
    String[] parts = StringUtility.split(str1, delim);
    Assert.assertEquals(Arrays.asList(str1), Arrays.asList(parts));
}

These new names describe well exactly what they are testing. This should help understanding the code.


The assertions can be simplified, instead of:

String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
Assert.assertTrue(parts.length == 3);
Assert.assertTrue(parts[0].equals(str1));
Assert.assertTrue(parts[1].equals(str2));
Assert.assertTrue(parts[2].equals(str3));

You could simplify like this:

String[] parts = StringUtility.split(str1 + delim + str2 + delim + str3, delim);
Assert.assertEquals(Arrays.asList(str1, str2, str3), Arrays.asList(parts));

This is exactly the same, but shorter, and a lot easier to write.


I would add some more test cases:

@Test
public void testOnlyDelim() {
    final String str1 = "";
    final char delim = ' ';
    String[] parts = StringUtility.split("" + delim + delim, delim);
    Assert.assertEquals(Arrays.asList(str1, str1, str1), Arrays.asList(parts));
}

@Test
public void testNotContainingDelim() {
    final String str1 = "hello";
    final char delim = 'x';
    String[] parts = StringUtility.split(str1, delim);
    Assert.assertEquals(Arrays.asList(str1), Arrays.asList(parts));
}

Suggested implementation

@heslacher's implementation is very close to my taste. Here's a variation of that, with some minor changes:

public static String[] split(String strToSplit, char delimiter) {
    List<String> arr = new ArrayList<>();
    int foundPosition;
    int startIndex = 0;
    while ((foundPosition = strToSplit.indexOf(delimiter, startIndex)) > -1) {
        arr.add(strToSplit.substring(startIndex, foundPosition));
        startIndex = foundPosition + 1;
    }
    arr.add(strToSplit.substring(startIndex));
    return arr.toArray(new String[arr.size()]);
}
\$\endgroup\$
0
4
\$\begingroup\$

The first thought that came to my mind has been why creating always a new StringBuilder after the delimiter is found. So I changed this and called the setLength() with zero as parameter like

public static String[] split(String strToSplit, char delimiter) {
    ArrayList<String> arr = new ArrayList<String>();
    StringBuilder sb = new StringBuilder();
    for (int i = 0; i < strToSplit.length(); i++) {
        char at = strToSplit.charAt(i);
        if (at == delimiter) {
            arr.add(sb.toString());
            sb.setLength(0);
        } else {
            sb.append(at);
        }
    }
    arr.add(sb.toString());
    return arr.toArray(new String[0]);
}

The next I thought, why use a StringBuilder and the CharAt(), if a indexOf() could be used, so I changed it to

public static String[] split(String strToSplit, char delimiter) {
    ArrayList<String> arr = new ArrayList<String>();
    int foundPosition = 0;
    int startIndex = 0;
    while ((foundPosition = strToSplit.indexOf(delimiter, startIndex)) >= 0) {

        arr.add(strToSplit.substring(startIndex, foundPosition));
        startIndex = foundPosition + 1;

    }
    arr.add(strToSplit.substring(startIndex));
    return arr.toArray(new String[0]);
}

You need to profile it by yourself.

\$\endgroup\$
0
2
\$\begingroup\$

I think it could be better to use substrings instead of adding one character at at time:

public static String[] split(String strToSplit, char delimiter) {
    ArrayList<String> arr = new ArrayList<String>();
    int start = 0;
    for (int i = 0; i < strToSplit.length(); i++) {
        if (strToSplit.charAt(i) == delimiter) {
            arr.add(strToSplit.substring(start,i));
            start = i+1;
        }
    }
    arr.add(strToSplit.substring(start));
    return arr.toArray(new String[0]);
}
\$\endgroup\$
0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.