Skip to content

updated the skeleton for Code Review Activity#14

Open
YukiRivera wants to merge 2 commits intomainfrom
yuki
Open

updated the skeleton for Code Review Activity#14
YukiRivera wants to merge 2 commits intomainfrom
yuki

Conversation

@YukiRivera
Copy link

No description provided.

@sarafarag sarafarag requested a review from puleri June 15, 2023 19:27
Copy link

@puleri puleri left a comment

Choose a reason for hiding this comment

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

Great work! You are passing all the test cases and I do not think anything needs changes. Feel free to review my notes but your work is awesome 👍🏻

String[] substring = Arrays.copyOfRange(words, startWord-1, endWord);

return substring;
}
Copy link

Choose a reason for hiding this comment

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

Great job on the getSubStrings method!

Appreciate how you've implemented the necessary validations and error handling. The code is clear and easy to understand, thanks to the informative exception messages you've provided. It helps users quickly identify and address any issues with their input.

Additionally, your use of the split() method to extract words from the string is a smart approach. It effectively handles various whitespace and punctuation characters, ensuring accurate word extraction.

Refactoring Opportunity:

One small refactoring opportunity I noticed is in the if condition for validating startWord and endWord. Instead of checking for startWord <= 0 || endWord <= 0, you could simplify it by using startWord < 1 || endWord < 1. This modification aligns with the subsequent comment explaining that zero values are invalid, and it enhances readability by removing unnecessary repetition.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your compliment and good suggestion! I really appreciate it:)

While I agree with you that (startWord < 1 || endWord < 1) looks simpler and might improve readability, I would go with my original code (startWord <= 0 || endWord <= 0) this time.

The reason is that it aligns with the requirement description. The requirement description for the method uses the following expressions:
* throws IllegalArgumentException
* If either "startWord" or "endWord" are invalid (i.e.,
* "startWord" <= 0, "endWord" <= 0, or "startWord"
* > "endWord")
Because of these expressions, I feel using the same expressions in the code would make it easier for anyone to check later if we have implemented the exception handling correctly.

Having said that, I know the requirement description would not be always clear like this and your suggestion definitely opened my eyes to other ways that could be used and possibly better in a lot of situations:)

String[] words = stringToManipulate.split("[\\s.(),:?!]+");

// returns the length of array as the word count
return words.length;
Copy link

Choose a reason for hiding this comment

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

Great work on the count() method!

I appreciate the clear and concise logic you've implemented. The conditional statements effectively handle scenarios where the stringToManipulate is null or empty, ensuring the method behaves correctly and avoids any potential issues.

I also commend your use of regular expressions in splitting the string into words. It's a clever approach that allows for flexibility in handling different symbols and punctuation marks. Regex is very complicated, yet very useful.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you, Matthew, for the compliment. I agree regex expressions are really useful in this type of situation :) I'm glad we had a chance to learn it in the CS program.

newString.append(stringToManipulate.charAt(indices[i]));
}
return newString.toString();
}
Copy link

Choose a reason for hiding this comment

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

Using a StringBuilder to construct the new string is a smart move. It helps optimize performance and memory usage, especially when dealing with multiple concatenations.

Here's a small suggestion for improvement: Consider creating a separate helper method to validate the indices. By isolating the index validation logic into its own method, you can enhance code readability and maintainability. Plus, if you need to reuse this logic elsewhere in your codebase, it'll be easier to do so.

Overall, your code shows a solid understanding of exception handling and effective string manipulation. You're doing an excellent job! If you have any questions or need further assistance, feel free to ask. Your contributions are highly valued, and keep up the great work!

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comment with insightful information! I did not know the StringBuilder helps the optimization on the performance and memory usage, I'm glad to learn that from you:)

And thank you for the suggestion that I could create a separate helper method. I agree it'd be a good idea for reusability and maintainability.

Based on your suggestion, I created a separate helper method to check if the string is null and if so, it throws an NullPointerException. I have the same check almost in every method so it's a good idea to separate it and reuse it.

For other checks, I decided to leave the code as-is because I feel my current code is clear in which exceptions are handled when validating indices within a particular method and each method has slightly different conditions for validation so I cannot really reuse them.

// nValue holds the length of the string plus one
int nValue = manipulatedstring.toString().length() + 1;
assertThrows(IndexOutOfBoundsException.class, () -> {manipulatedstring.removeNthCharacter(nValue, false);});
}
Copy link

Choose a reason for hiding this comment

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

Good looking test:
Here's another approach you could take as well.

@Test
public void testRemoveNthCharacter5() {
    manipulatedstring.setString("I live in Seattle.");
    int nValue = manipulatedstring.toString().length() + 1;
    
    try {
        manipulatedstring.removeNthCharacter(nValue, false);
        fail("Expected IndexOutOfBoundsException to be thrown.");
    } catch (IndexOutOfBoundsException e) {
        // Asserting that the exception message contains relevant information
        assertTrue(e.getMessage().contains("Invalid character index"));
    }
}

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for telling me how to use a different approach. I will keep my test case as-is this time because it works as expected, but I will keep in mind that I could use an approach like this in the future:)

@YukiRivera
Copy link
Author

YukiRivera commented Jun 17, 2023

I made changes and added a helper function based on the suggestion.

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