Thomas/Paige Riffle Code Review Pull Request#33
Thomas/Paige Riffle Code Review Pull Request#33Trifflenator wants to merge 1 commit intoSWE-CS410:mainfrom
Conversation
kemeng001
left a comment
kemeng001
left a comment
There was a problem hiding this comment.
The implementation and testing are generally very good, but there is still some room for improvement to make the code more robust.
| public int count() { | ||
| return 0; | ||
| //checks for null or 'empty' one char strings, anything more is a bit beyond scope of project | ||
| if( str == null || str == " ") { |
There was a problem hiding this comment.
More exceptions may make this method more robust, for example, handling strings that contain only spaces, or strings that contain punctuation and other non-alphabetic characters.
| return 0; | ||
| //checks for null or 'empty' one char strings, anything more is a bit beyond scope of project | ||
| if( str == null || str == " ") { | ||
| return 0; |
There was a problem hiding this comment.
In other methods, invalid inputs lead to an IllegalArgumentException, to shows the user why this error happens, we may need similar Exception in this method, to make user understand why this input is illegal.
| return null; | ||
| public String restoreString(int[] indices){ | ||
| //Exception for incorrect int array | ||
| if(indices.length != str.length()) { |
There was a problem hiding this comment.
Check whether indices is null before using may make the code more robust, or when try to use indices.length may cause NullPointerException.
| Exception exception = assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.getSubStrings(0, 1);}); | ||
| String expected = "startWord must be greater than zero"; | ||
| String actual = exception.getMessage(); | ||
| assertTrue(actual == expected); |
There was a problem hiding this comment.
Other test cases use assertEquals to check the actual and expected value, but this one uses assertTrue, in this way, we cannot see the difference between the actual and expected value.
There was a problem hiding this comment.
This implementation basically meets the performance and functional requirements, but still needs to be improved in some places to achieve better robustness.
spadeX6
left a comment
spadeX6
left a comment
There was a problem hiding this comment.
Implementation is good overall, approving!
| @Override | ||
| public String[] getSubStrings(int startWord, int endWord) { | ||
| return null; | ||
| public String[] getSubStrings(int startWord, int endWord){ |
There was a problem hiding this comment.
Regarding Exception Handling: This method is doing a great job of validating its input parameters and throwing exceptions accordingly. However, the error messages could be made a little more user-friendly, perhaps by including the invalid value in the message itself for quick reference.
On String Concatenation: In the loop where words are constructed using result[words] += str.charAt(x);, it's important to remember that strings in Java are immutable and that each concatenation results in the creation of a new object. This could potentially be inefficient in terms of memory usage, especially for long strings. Consider using a StringBuilder for constructing the words, which would be more efficient.
On Loop Control Variable 'x': The variable 'x' is being used to track the index in the original string. It would be helpful to provide a more descriptive name for this variable to make the code more readable.
On Variable Naming: The variables startWord, endWord, and words are well named and convey their purpose effectively. However, x and i could be renamed to something more descriptive to further improve readability. For example, x could be renamed to charIndex and i could be renamed to wordIndex. This could help other developers understand the code more quickly.
3 participants
Late due to family affairs yesterday and today.