Conversation
StringManipulation.Java and StringManipulationtest
Trev10
left a comment
Trev10
left a comment
There was a problem hiding this comment.
Hey Igor you did a very good job overall I don't really see any big issues other than just some minor changed needed please make sure you check my comments and feel free to let me know if you need any clarifications.
| fail("Not yet implemented"); | ||
| manipulatedstring.setString(""); | ||
| int length = manipulatedstring.count(); | ||
| assertEquals(1, length); |
There was a problem hiding this comment.
I think this should be 0 since the string is empty I will leave a suggestion on your count() in the StringManuoulation to fix this problem.
| @Override | ||
| public int count() { | ||
| return 0; | ||
| String string = getString(); |
There was a problem hiding this comment.
To make sure that if there's an empty sting it will return a 0 you can add a if statement to it so that if the string is empty it will return 0 (using methods like isEmpty())
|
|
||
| assertThrows( | ||
| IndexOutOfBoundsException.class, () -> { | ||
| manipulatedstring.removeNthCharacter(100, true); |
There was a problem hiding this comment.
I don't think it matters if the maintainSpaceing is true or false in this situations since 100 is way larger than the string length so I'll say you can possibly make it another test because in testRemoveNthCharacter3 you already tested the same thing
There was a problem hiding this comment.
I prefer to treat code like black box and test incase that true or false can result in difference case of exceptions
|
|
||
| assertThrows( | ||
| IllegalArgumentException.class, () -> { | ||
| manipulatedstring.removeNthCharacter(0, true); |
There was a problem hiding this comment.
This case is the same since n cannot be 0 then it should always throw the exception so I think you can change it to another test. Just so you have an idea you can maybe test when n is 1 since I didn't see you put that in a test.
Trev10
left a comment
Trev10
left a comment
There was a problem hiding this comment.
Hey Igor I just finished looking at all changed and it's fine that you want to keep the maintain spacing true and false thing which is understandable and I see you fixed the minor issue so I think you are all set good job.
2 participants
StringManipulation.Java and
StringManipulationtest