Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

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
share|improve this question
1  
Just to be sure, you are looking to reinvent the wheel, right? Because String#split already exists. –  skiwi Aug 5 at 15:45
    
Java 5 gave us the enhanced for loop. And Java 8 gives us streams - why are you programming in Java 4? –  Martin Schröder Aug 6 at 12:56
    
@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. –  JaDogg Aug 6 at 16:42
1  
@skiwi I've added the reinvent the wheel tag –  JaDogg Aug 24 at 12:00

3 Answers 3

up vote 5 down vote accepted

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()]);
}
share|improve this answer

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.

share|improve this answer

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]);
}
share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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