updated w/ final implementation and tests#9
Conversation
There was a problem hiding this comment.
General comment: I think your methods are both very concise and your variables names are chosen good. It was difficult to find glaring issues in your logic. There were a few edge cases not considered in your test file and I made comments that addressed those topics. Great job, especially on the part where you checked for duplicates by making a helper method using the set method.
|
|
||
| public class StringManipulation implements StringManipulationInterface { | ||
|
|
||
| String s; |
There was a problem hiding this comment.
On line 8, the data field String s should be private. The business logic of the code needs to incorporate encapsulation to protect from external access.
| public String removeNthCharacter(int n, boolean maintainSpacing) { | ||
| return null; | ||
| if (n <= 0) { | ||
| throw new IllegalArgumentException(); |
There was a problem hiding this comment.
It may be helpful to provide am message pertaining to the exact reason illegalArgumentException is called. This may help clarify during testing to save some time.
| public String restoreString(int[] indices) { | ||
| return null; | ||
| public String restoreString(int[] indices){ | ||
| if(indices.length != s.length() || duplicates(indices)){ |
There was a problem hiding this comment.
Utilizing a set here to check for duplicates is nice way and optimal way to compare for duplicates. You helped me realize that I might not have done this for my own method. Well done.
| array=new int[]{1,0,2,3}; | ||
| assertThrows(IllegalArgumentException.class, () -> manipulatedstring.restoreString(array)); | ||
| } | ||
|
|
There was a problem hiding this comment.
Your code handles illegalArgumentException in two situations; the first is when there are duplicates, and the second when the size of the array is either too big or too small relative to the length of the string. I see you testing the duplicates well, but what about different length of arrays being inputted? Another testRestoreString6 could be here.
| } | ||
|
|
||
| //This test handles IllegalArgumentException for improper start and end values | ||
| @Test |
There was a problem hiding this comment.
This test case is redundant. The test above tests the exact same thing as below. you're Testing the situation where the startWord is greater than the endWord, but there is also the possibility the start word and end words are the same, OR that the parameter inputs a negative number on accident. These are other cases you can test.
2 participants
No description provided.