patternjavaMinor
Critique of Delimited String Method
Viewed 0 times
critiquestringdelimitedmethod
Problem
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:
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:
Supporting tests:
```
@Test
public void testDelimitedStringsSimple() throws Exception {
String[] strArray = {""};
assertEquals(strArray, StringUtil.delimitedStrings("I say ", ''));
}
@Test
public void testDelimitedStringsComplex() throws Exception {
String[] strArray = {"","","", ""};
assertEquals(strArray, StringUtil.delimitedStrings("I say I say and a tricky and >", ''));
}
@Test
public void testDelimitedStringsNoStart() throws Exception {
String[] strArray = {};
assertEquals(strArray, StringUtil.delimitedStrings("Bonjour!>", ''));
}
@Test
public void testDelimitedStringsNoEnd() throws Exception {
Stri
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 > 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 list = new ArrayList();
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 = {""};
assertEquals(strArray, StringUtil.delimitedStrings("I say ", ''));
}
@Test
public void testDelimitedStringsComplex() throws Exception {
String[] strArray = {"","","", ""};
assertEquals(strArray, StringUtil.delimitedStrings("I say I say and a tricky and >", ''));
}
@Test
public void testDelimitedStringsNoStart() throws Exception {
String[] strArray = {};
assertEquals(strArray, StringUtil.delimitedStrings("Bonjour!>", ''));
}
@Test
public void testDelimitedStringsNoEnd() throws Exception {
Stri
Solution
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
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.
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
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.
this allows me to rewrite my while method to this.
```
public static String[] split(String from, char startDelimiter, char endDelimiter)
{
ArrayList list = new ArrayList<>();
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
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 list = new ArrayList<>();
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 list = new ArrayList<>();
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 && 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 list = new ArrayList<>();
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
Code Snippets
public static String[] split(String from, char startDelimiter, char endDelimiter)
{
int startPos = 0;
ArrayList<String> list = new ArrayList<>();
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()]);
}public static String[] split(String from, char startDelimiter, char endDelimiter)
{
int fromIndex = 0;
ArrayList<String> list = new ArrayList<>();
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;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 && endPosistion != NOTFOUND);
}public static String[] split(String from, char startDelimiter, char endDelimiter)
{
ArrayList<String> list = new ArrayList<>();
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 && endPosistion != NOTFOUND);
}
private static final int NOTFOUND = -1;public static String[] split(String from, char startDelimiter, char endDelimiter)
{
ArrayList<String> list = new ArrayList<>();
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 && endPosistion != NOTFOUND);
}
private static final int NOTFOUND = -1;Context
StackExchange Code Review Q#32926, answer score: 2
Revisions (0)
No revisions yet.