Conversation
BladeTheHunter005
left a comment
BladeTheHunter005
left a comment
There was a problem hiding this comment.
Project accepted. Code review successfully completed
| @@ -1,32 +1,79 @@ | |||
| public class StringManipulation implements StringManipulationInterface { | |||
| private String myString = ""; | |||
|
|
|||
There was a problem hiding this comment.
Great job creating the StringManipulation class and implementing the StringManipulation Interface.
| public String getString() { | ||
| return null; | ||
| return myString; | ||
| } |
There was a problem hiding this comment.
It's good to see the getString() method implemented, which returns the value of myString. It allows users to retrieve the current string.
| @Override | ||
| public void setString(String string) { | ||
| myString = string; | ||
| } |
There was a problem hiding this comment.
Method effectively sets the value of myString to the provided string.
| } | ||
| } | ||
| return i; | ||
| } |
There was a problem hiding this comment.
Have you considered using the enhanced for-loop syntax for the count() method to improve readability? For example,
for(String str: spl) instead of using traditional for-loop.
To make the code even more readable, could you add a comment explaining what the purpose of the count() method is?
I know we all know what it is and what it does, but good practice is documenting everything.
| } | ||
| } | ||
| return ret.toString(); | ||
| } |
There was a problem hiding this comment.
This removeNthCharacter(int n, boolean maintainSpacing) method looks good.
It is good to see you handling cases where n is greater than myString.length() and when n is less than or equal to 0.
Once again have you consider commenting detailed documentation so that your code is easy to modify. Remember in the verification and validation phase of testing that we have to find bugs, ensure readability. Consider documenting everything.
| public void testRemoveNthCharacter1() { | ||
| manipulatedstring.setString("I'd b3tt3r put s0me d161ts in this 5tr1n6, right?"); | ||
| assertEquals("I' bttr uts0e 16tsinths trn6 rgh?", manipulatedstring.removeNthCharacter(3, false)); | ||
| } |
There was a problem hiding this comment.
testing the removal of the third character is a good test case.
| @@ -87,27 +130,66 @@ public void testGeSubStrings1() { | |||
| assertEquals(sStings[1], "string"); | |||
| } | |||
There was a problem hiding this comment.
you're testing the splitting of a string into substrings. This is a good test case. Have you considered what might happen if the string contains multiple spaces between words?
| assertEquals(sStings[0], "my"); | ||
| assertEquals(sStings[1], "string"); | ||
| } | ||
|
|
There was a problem hiding this comment.
This is a good test case. Have you considered what might happen if the string contains other types of whitespace characters, like tabs or newlines?
| * of the spec, but my implementation does it anyway and I wanted to test it.) | ||
| */ | ||
| @Test | ||
| public void testRestoreString5() |
There was a problem hiding this comment.
And this one, you're throwing in non-alphanumeric characters into the mix. Real-world strings can be messy and full of all sorts of characters, so it's awesome that you're making sure your method can handle that
| * the number of letters in the string. | ||
| */ | ||
| @Test | ||
| public void testRestoreString3() |
There was a problem hiding this comment.
Here, you're testing if your method can deal with a mismatch in the number of indices and the number of characters in the string. It's like trying to fit a square peg in a round hole - it's not gonna work, and your method should recognize that. Good on you for checking this!
2 participants
No description provided.