Critique of Delimited String Method I'm working my way through The Java Programming Language, Fourth Edition - The Java Series.  This is Exercise 13.3:


  As shown, the delimitedString method assumes only one such string per input string. Write a version that will pull out the delimited strings and retun an array.


The provided delimitedString method:

public static String delimitedString(String from, char start, char end) {
    int startPos = from.indexOf(start);
    int endPos = from.lastIndexOf(end);

    if (startPos == -1)
        return null;
    else if (endPos == -1)
        return null;
    else if (startPos &gt; endPos)
        return null;
    else
        return from.substring(startPos, endPos + 1);
}


My solution. Note that regular expressions are covered in the next section of the book, so I chose not to use a split for this particular exercise:

public static String[] delimitedStrings(String from, char startDelim, char endDelim) {
    int index;
    int startPos = 0;
    ArrayList&lt;String&gt; list = new ArrayList&lt;String&gt;();

    while ((index = from.indexOf(startDelim, startPos)) != -1) {
        int endPos;
        if ((endPos = from.indexOf(endDelim, index)) != -1) {
            list.add(from.substring(index, endPos + 1));
            startPos = endPos;
        } else {
            break;
        }
    }
    return list.toArray(new String[list.size()]);
}


Supporting tests:

@Test
public void testDelimitedStringsSimple() throws Exception {
    String[] strArray = {"&lt;Bonjour!&gt;"};
    assertEquals(strArray, StringUtil.delimitedStrings("I say &lt;Bonjour!&gt;", '&lt;', '&gt;'));
}

@Test
public void testDelimitedStringsComplex() throws Exception {
    String[] strArray = {"&lt;Bonjour!&gt;","&lt;Bonjour!&gt;","&lt;B&lt;onjour!&gt;", "&lt;Bonjour!&gt;"};
    assertEquals(strArray, StringUtil.delimitedStrings("I say &lt;Bonjour!&gt; I say &lt;Bonjour!&gt; and a tricky &lt;B&lt;onjour!&gt; and &gt;&lt;Bonjour!&gt;", '&lt;', '&gt;'));
}

@Test
public void testDelimitedStringsNoStart() throws Exception {
    String[] strArray = {};
    assertEquals(strArray, StringUtil.delimitedStrings("Bonjour!&gt;", '&lt;', '&gt;'));
}

@Test
public void testDelimitedStringsNoEnd() throws Exception {
    String[] strArray = {};
    assertEquals(strArray, StringUtil.delimitedStrings("&lt;Bonjour!", '&lt;', '&gt;'));
}

@Test
public void testDelimitedStringsReversed() throws Exception {
    String[] strArray = {};
    assertEquals(strArray, StringUtil.delimitedStrings("&gt;Bonjour!&lt;", '&lt;', '&gt;'));
}

@Test
public void testDelimitedStringsEmpty() throws Exception {
    String[] strArray = {};
    assertEquals(strArray, StringUtil.delimitedStrings("", '&lt;', '&gt;'));
}
If I had to put a number on your code it would be a 8 out of 10. You did good here. Since it is so good I am going to be nitpicking a few things. First is your test. You are comparing arrays but using assertEquals. Instead you need to change that to assertArrayEquals (providing you are using JUnit 4.0) The other thing I feel you need to test for is no begin and no end delimiters.

The other nitpicking thing I can find is in your split method. I know that you referred to not using the String.split method, but your method does the same thing. For this exercise there is nothing wrong with giving it the same name seeing as it does the same thing. One thing that annoys me a small amount is the making a while or if statement do more than one thing. Are you assigning or are you comparing? The convention for while and if is to compare only. So move your assignment out of those functions. I'm going to paste your code multiple times and I revise it, so here is where I am sitting now on your code.

public static String[] split(String from, char startDelimiter, char endDelimiter)
{
    int startPos = 0;
    ArrayList&lt;String&gt; list = new ArrayList&lt;&gt;();

    int index = from.indexOf(startDelimiter, startPos);

    while (index != -1)
    {
        int endPos = from.indexOf(endDelimiter, index);
        if (endPos != -1)
        {
            list.add(from.substring(index, endPos + 1));
            startPos = endPos;
        }
        else
        {
            break;
        }
        index = from.indexOf(startDelimiter, startPos);
    }
    return list.toArray(new String[list.size()]);
}


Notice that it introduces some duplication with assigning index. Since you have passing tests that means we can refactor a little bit. Now I know you are probably thinking that the reason you put the assignment in the while loop was to remove duplication. Before we address that though lets attack some of your naming. Index. Normally this is a good choice of words to use, but index usually refers to a location in an array. Although a string is an array of characters in this context it is not the exact definition of an index. So I believe a more appropriate name would be startDelimiterLocation , startLocation , or even startLookigFrom this would allow us to make a sentence out of our while and if statments by giving -1 a name such as NotFound or since it is a constant NOTFOUND.

public static String[] split(String from, char startDelimiter, char endDelimiter)
{
    int fromIndex = 0;
    ArrayList&lt;String&gt; list = new ArrayList&lt;&gt;();

    int startPosistion = from.indexOf(startDelimiter, fromIndex);

    while (startPosistion != NOTFOUND)
    {
        int endPosistion = from.indexOf(endDelimiter, startPosistion);
        if (endPosistion != NOTFOUND)
        {
            list.add(from.substring(startPosistion, endPosistion + 1));
            fromIndex = endPosistion;
        }
        else
        {
            break;
        }
        startPosistion = from.indexOf(startDelimiter, fromIndex);
    }
    return list.toArray(new String[list.size()]);
}
private static final int NOTFOUND = -1;


This gives us the ability to read the code outloud if we wanted (while startPosistion is not not found) (yes i know it's a double negative, but well.. this isn't English. Well that gives me an idea.. Lets make that into an actual sentance, and have it return a boolean.

private static boolean stringHasStringBetweenTwoCharacters(String from, char startDelimiter, char endDelimiter)
{
    int startPosistion = from.indexOf(startDelimiter);
    if (startPosistion == NOTFOUND) return false;

    int endPosistion = from.indexOf(endDelimiter, startPosistion);
    return (startPosistion != NOTFOUND &amp;&amp; endPosistion != NOTFOUND);
}


this allows me to rewrite my while method to this. while (stringHasStringBetweenTwoCharacters(from, startDelimiter, endDelimiter)) but this makes the tests fail because my method is not looking at where we left off in our string. I removed it on purpose because I wanted this method to only find the first occurrence of a string in between 2 characters. So this means I have to change the logic slightly. Instead of keeping track of where we are in the string, lets just remove up to the endPosistion and leave the rest.

public static String[] split(String from, char startDelimiter, char endDelimiter)
{
    ArrayList&lt;String&gt; list = new ArrayList&lt;&gt;();

    while (stringHasStringBetweenTwoCharacters(from, startDelimiter, endDelimiter))
    {
        int startPosistion = from.indexOf(startDelimiter);
        int endPosistion = from.indexOf(endDelimiter, startPosistion);
        if (endPosistion != NOTFOUND)
        {
            list.add(from.substring(startPosistion, endPosistion + 1));
            from = from.substring(endPosistion + 1);
        }
        else
        {
            break;
        }
    }
    return list.toArray(new String[list.size()]);
}
private static boolean stringHasStringBetweenTwoCharacters(String from, char startDelimiter, char endDelimiter)
{
    int startPosistion = from.indexOf(startDelimiter);
    if (startPosistion == NOTFOUND) return false;

    int endPosistion = from.indexOf(endDelimiter, startPosistion);
    return (startPosistion != NOTFOUND &amp;&amp; endPosistion != NOTFOUND);
}
private static final int NOTFOUND = -1;


so now my tests pass again. Oh but I have duplication again, but no longer in the start position, now it is in the end position. Since I know it is there I can remove the check in my while loop and make a slight adjustment in my duplicate change in index. My final outcome is something like this.

public static String[] split(String from, char startDelimiter, char endDelimiter)
{
    ArrayList&lt;String&gt; list = new ArrayList&lt;&gt;();

    while (stringHasStringBetweenTwoCharacters(from, startDelimiter, endDelimiter))
    {
        int startPosistion = from.indexOf(startDelimiter);
        int endPosistion = from.indexOf(endDelimiter, startPosistion) + 1;
        list.add(from.substring(startPosistion, endPosistion));
        from = from.substring(endPosistion);
    }
    return list.toArray(new String[list.size()]);
}
private static boolean stringHasStringBetweenTwoCharacters(String from, char startDelimiter, char endDelimiter)
{
    int startPosistion = from.indexOf(startDelimiter);
    if (startPosistion == NOTFOUND) return false;

    int endPosistion = from.indexOf(endDelimiter, startPosistion);
    return (startPosistion != NOTFOUND &amp;&amp; endPosistion != NOTFOUND);
}
private static final int NOTFOUND = -1;


Now my methods only do one thing, and it does them well. I moved the logic of how to check to see if my string still contains a token into its own method which leave only the logic of adding my tokens to my arrayList. It may seem a little more verbose, but in my opinion is easier to read