Conversation
Yusa-Kaya
left a comment
Yusa-Kaya
left a comment
There was a problem hiding this comment.
Overall, really good work.
Travis builds fine and test cases look thorough.
I hope my feedback was helpful! Good luck.
|
|
||
| public class StringManipulation implements StringManipulationInterface { | ||
|
|
||
| public String stored; |
There was a problem hiding this comment.
It is better to have fields as "private" since you already have getString and setString methods.
| //make a char array of the stored string | ||
| char[] newString = new char[indices.length]; | ||
| for(int i = 0; i < indices.length; i++) { | ||
| if(indices[i] < 0 || indices[i] > stored.length()) { |
There was a problem hiding this comment.
indices[i] should be greater than or equal to stored.length() (indices[i] >= stored.length()), so that you can avoid indexoutofbound exception.
| boolean inSelection = false; | ||
| ArrayList<String> returns = new ArrayList<String>(); | ||
|
|
||
|
|
There was a problem hiding this comment.
since you already validated your interval (startWord to endword), and split the string by space character, I think you can simply return a copy of "words" array. How about "return Arrays.copyOfRange(words, startWord - 1, endWord);?" Do you think this would work?
| } | ||
|
|
||
| String result; | ||
|
|
There was a problem hiding this comment.
I used StringBuilder which is an efficient way of handling string manipulations especially when dealing with loops. I think you can just use a StringBuilder object to avoid conversions from string to char array to arraylist and back.
There was a problem hiding this comment.
Here is how you can do that:
// Create a mutable string (StringBuilder in Java) from the input string
StringBuilder = create a mutable string from stored
// Loop through the multiples of n
for i from 1 until n * i <= stored.length(); i++:
// Calculate the index of the character to be removed/replaced. Since indexing is 0 bases, subtract 1.
index = (n * i) - 1
// If maintainSpacing is true, replace the character at the position with a space
if maintainSpacing is true:
set character at position index in StringBuilder to ' '
// If maintainSpacing is false, remove the character at the position
else:
// Decrease index by i-1 to account for previously deleted characters
delete the character at position (index - (i - 1)) in StringBuilder
// Convert the mutable string back to a regular string and return it
return convert StringBuilder to string
| assertEquals(1, length); | ||
| } | ||
|
|
||
| //IndexOB test |
There was a problem hiding this comment.
Maybe you can give more information about what these cases are actually testing.
There was a problem hiding this comment.
Also, you could rename test cases so that they have descriptive names indicating what they are testing.
| public void testGeSubStrings5() { | ||
| fail("Not yet implemented"); | ||
| assertThrows(IllegalArgumentException.class, () -> { | ||
| manipulatedstring.setString("bad test!"); |
There was a problem hiding this comment.
Maybe you could have more descriptive assert messages?
2 participants
:)