Skip to content

Added StringManipulation.java and StringManipulationTest.java files#3

Open
Yusa-Kaya wants to merge 2 commits intomainfrom
Yusa_Kaya_CodeReview
Open

Added StringManipulation.java and StringManipulationTest.java files#3
Yusa-Kaya wants to merge 2 commits intomainfrom
Yusa_Kaya_CodeReview

Conversation

@Yusa-Kaya
Copy link

No description provided.

@sarafarag sarafarag requested a review from EllisLempriere June 14, 2023 18:18
Copy link

@EllisLempriere EllisLempriere left a comment

Choose a reason for hiding this comment

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

Travis passing
Solid test cases with good coverage
Clean readable code implementation

assertEquals(expected, actual);
}

// Throw IllegalArgumentException if the given index is less than or equal to 0
Copy link

@EllisLempriere EllisLempriere Jun 14, 2023

Choose a reason for hiding this comment

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

Looks like this comment is swapped with the comment on line 95, would recommend swapping for less confusion

Copy link
Author

Choose a reason for hiding this comment

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

Ups, thanks for pointing that out! Copying and pasting can be risky :)

assertEquals(0, length);
}

// Test when string is null

Choose a reason for hiding this comment

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

I like that you added comments describing a bit about the test. Try looking into the @DisplayName annotation for an addition nice way to format test details

Copy link
Author

Choose a reason for hiding this comment

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

Oh, that's really cool. I'll use that

manipulatedstring.restoreString(indices);});
}

// Test when each character is the same in the string

Choose a reason for hiding this comment

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

This is an interesting test case, I wonder in what ways it would break/why it would be needed

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it is an interesting test case. I am not sure how you would break this case too. I run out of ideas and made up a case lol. Maybe it could be useful when implementation takes shortcuts or makes assumptions if the characters are the same. This test case ensures that the code does not make invalid assumptions.

assertEquals(4, length);
}

// Test when string has multiple punctuations.

Choose a reason for hiding this comment

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

Testing against the punctuation is a good idea. I can see how this could easily break some implementations of the method

Copy link
Author

Choose a reason for hiding this comment

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

You are right. If the test case was "This isn't my string !" and they put a space before the exclamation point it will break the test case because my method counts punctuations as a word too. My method implementation is way too simple to catch that. I think it would be better if I refactor that method and make it ignore punctuation.

…hat it considers punctuation and numerical values. Added 2 new test cases to test the newly added part.
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