Skip to content

celine_chiou updated skeleton with my code for peer review#17

Open
chiouc wants to merge 1 commit intomainfrom
Celine_Chiou_CodeReview
Open

celine_chiou updated skeleton with my code for peer review#17
chiouc wants to merge 1 commit intomainfrom
Celine_Chiou_CodeReview

Conversation

@chiouc
Copy link

@chiouc chiouc commented Jun 14, 2023

No description provided.

@sarafarag sarafarag requested a review from KhalilxCoats June 15, 2023 16:35
Copy link

@KhalilxCoats KhalilxCoats left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code is functional, neat, and efficient. One of the test cases could be a bit more thorough in checking for exceptions, but overall there don't seem to be any major issues.


public class StringManipulation implements StringManipulationInterface {
private String currentString;
private int count;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

given that the word count value is only used when called by the count() function, would it not be better to have count be local to the count function as opposed to to having it as a private data member.

String trimmedString = currentString.trim().replaceAll("\\s+", " ");
String[] wordArray = trimmedString.split(" ");

// throws IndexOutOfBoundsException If the string has less than "endWord" words in it

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be neater if you placed this code with the other exception if-statement so all the code that handles exceptions for this method are in one place, maybe even as an else-if statement as opposed to a completely separate if-statement.

@Test
public void testGeSubStrings2() {
fail("Not yet implemented");
manipulatedstring.setString("This is my test string sentence that should only return a few words");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Somewhat redundant test case since it does the same thing as the first test.

public void testGeSubStrings4() {
fail("Not yet implemented");
manipulatedstring.setString("This is my test string");
assertThrows(IllegalArgumentException.class, () -> {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any way to ensure that this test case is throwing the illegal argument exception in response to the negative number argument and not because the start word is greater than the end word, given that both throw the same exception type.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants