Conversation
There was a problem hiding this comment.
Aravind,
Very nice start on your code.
The only branch in your code that was missed with a test case was the getSubstring() return null, which I already mentioned. The other big issue I found was marking your test cases with "throws Exception". These is the only issues I would explicitly say that you should fix before merging.
Other than that, I would suggest adjusting your getSubstring() method to drop punctuations from words you extract, adjusting your count() method to exclude "words" made of punctuations (!!!! !!!! ----), and maybe changing your methods to throw a NullPointerException when called on a null string. These are dependent on the client's requirements though.
| @@ -20,66 +27,93 @@ public void tearDown() { | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Aravind, you're missing a requirement in your tests to verify that getString() is returning null when the string is null.
| This file contains the implementations for the string manipulations class. | ||
| */ | ||
| public class StringManipulation implements StringManipulationInterface { | ||
|
|
There was a problem hiding this comment.
No issues with the build on TravisCI.
| @Override | ||
| public String removeNthCharacter(int n, boolean maintainSpacing) { | ||
| return null; | ||
| if(current == null || current.isEmpty()) |
There was a problem hiding this comment.
Great job checking for current == null before trying to call a method on the null string.
It would error if you had a null string and current.isEmpty() came before current == null in your conditions.
| int start = startWord - 1; //the given ints are always 1 ahead since they start counting from 1 | ||
| int end = endWord - 1; | ||
|
|
||
| String[] words = current.trim().split(" "); //removes white space at the beginning and end of the string and splits by whitespace |
There was a problem hiding this comment.
I don't see any issue with using this method to break up the words in a String, but instead of instantiating another array and filling it, you could use a regex to extract and populate the output array directly.
| @Override | ||
| public int count() { | ||
| return 0; | ||
| if(current == null || current.isEmpty()) //empty or null string have a length of 0 |
There was a problem hiding this comment.
You may want to consider throwing a NullPointerException when count, or any of your other methods are called on a null string instead of returning something.
|
|
||
| String newStr = ""; | ||
| for(int i = 0; i < indices.length; i++) { | ||
| if(indices[i] < 0 || indices[i] > current.length() - 1) { |
There was a problem hiding this comment.
Something minor:
I think it's a little clearer if you use "indices[i] >= current.length()" instead of "indices[i] > current.length() - 1".
| int length = manipulatedstring.count(); | ||
| assertEquals(7, length); | ||
| } | ||
|
|
There was a problem hiding this comment.
I don't see a test case for strings that look like "!!!!! !!!!". Your count method would return 2 words in this case and a group of exclamation points is not a word.
| assertEquals(7, length); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
You have quite a few of your test methods marked "throws Exception." This is unnecessary. You should remove the throws statement in the method declaration.
There was a problem hiding this comment.
As you can see from my Intellij, it doesn't recognize the method as throwing an exception.
The test cases will also run just fine without the throws Exception statement.
| fail("Not yet implemented"); | ||
| //this test check for correct output by leaving the string with its default value | ||
| public void testRemoveNthCharacter3() throws Exception { | ||
| assertEquals(null, manipulatedstring.removeNthCharacter(3, false)); |
There was a problem hiding this comment.
Instead of "assertEquals" you should use "assertNull" here.
Another option is (as mentioned in the StringManipulation file) is set your methods to throw a NullPointerException when your string field is null. In this case you can assertThrows(NullPointerException.class, ....).
| manipulatedstring.setString("CS4420"); | ||
| assertEquals("CS420", manipulatedstring.removeNthCharacter(4, false)); | ||
| } | ||
|
|
There was a problem hiding this comment.
One note on the getSubstrings() method:
I notice that you don't have test cases that include punctuation, like "I need some sleep!". The reason I mention this is that I can tell by the way you split strings, the "!" will be included with the string. I'd recommend using a regex to extract your words, or additional processing on the temp array you use to hold strings to remove punctuations.
2 participants
My code to be reviewed!