Conversation
| return null; | ||
|
|
||
| if(s == null){ | ||
| throw new NullPointerException("string cannot be null"); |
There was a problem hiding this comment.
Null ptr exception not mentioned in the interface for the assigment.
Throw indexoutofbound? null str has no words
There was a problem hiding this comment.
Yeah you are probably right, I just wanted to add exception checking and figured that a string might be null so why not check whether the string was nulll.. indexOutOfBounds is a good suggestion
|
|
||
| char[] shuffled = new char[s.length()]; | ||
|
|
||
| for (int i = 0; i < s.length(); i++) { |
There was a problem hiding this comment.
move exception checking up? save on shuffling in event that exception is thrown later? idk may not be computationally significant i'm just writing words
| @@ -20,66 +29,87 @@ public void tearDown() { | |||
| } | |||
|
|
|||
| @Test | |||
There was a problem hiding this comment.
what if string contains non alphanumeric chars? no test case but maybe that's not even in the scope of this assignment who knows?
There was a problem hiding this comment.
That edge case was out of the bounds of the assignment.
Future improvement for other classes for sure
| } | ||
|
|
||
| @Override | ||
| public void setString(String string) { |
There was a problem hiding this comment.
In the event that user sets string = "", consider overriding that input with null? May be outside of how Sara expects us to implement, though perhaps worth considering for methods below. Save on calling isEmpty(). For sure up for interpretation, no single answer.
|
|
||
| @Test | ||
| public void testCount1() { | ||
| public void testCount1_NormalString() { |
There was a problem hiding this comment.
count() test cases provided for alphabet/empty/null strings, further test cases by Sara suggest string provided may include numbers interspersed with letters, consider adding a test case for this scenario.
Edit: I realized that this comment is just rehashing what I wrote in the comment above this one, but I will leave it up anyways.
| String [] sStings = manipulatedstring.getSubStrings(1, 5); | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
All RestoreString() test cases use the same string, consider adding tests that deal with numbers/symbols/whitespace.
| int length = manipulatedstring.count(); | ||
| assertEquals(4, length); | ||
| } | ||
|
|
There was a problem hiding this comment.
How should removeNthCharacter(1, true/false) behave? Should the method return "" or null?
| ()-> { | ||
| String restoreString = manipulatedstring.restoreString(array); | ||
| }); | ||
| } |
There was a problem hiding this comment.
With the exception of testCount(), all of the test cases check if exceptions are thrown. It may be worth adding unique test cases that test your program logic to ensure that your methods operate as you intend them to.
|
|
||
| public class StringManipulation implements StringManipulationInterface { | ||
|
|
||
| public String s; |
There was a problem hiding this comment.
Changing String from public to protected/private here would increase code encapsulation.
| throw new IndexOutOfBoundsException("n is greater than the string length."); | ||
| } | ||
|
|
||
| StringBuilder sb = new StringBuilder(); |
There was a problem hiding this comment.
Here you can instantiate StringBuilder() by passing an existing string to it. After, you can call StringBuilder.remove(int index) to remove characters at a given position. If maintainSpacing == true, StringBuilder.setCharAt(int index, char ch) can be called to replace chars with ' '.
There was a problem hiding this comment.
Hi Jon, I've added more comments in addition to the those added 6/16. Overall, your code is clean and easy to follow, with most of my comments reading more as differences of opinion than suggestions to change anything. I especially like how you leverage the java.util library to accomplish the objectives of the assignment. After the C++ we learned in past classes, I forgot how important builtin libraries are. While some may critique this as unnecessary code coupling, I truly see it as an excellent example of code reuse, saving dev time. The one major recommendation I have is to add more variety to the test cases. Otherwise, great work!
2 participants
No description provided.