Skip to content

Added new versions of StringManipulation .java files#6

Open
AaronBird759 wants to merge 1 commit intomainfrom
Aaron_Bird_CodeReview
Open

Added new versions of StringManipulation .java files#6
AaronBird759 wants to merge 1 commit intomainfrom
Aaron_Bird_CodeReview

Conversation

@AaronBird759
Copy link

No description provided.

@sarafarag sarafarag requested a review from Emc2Ikeda June 14, 2023 18:21
@Test
public void testCount4() {
fail("Not yet implemented");
manipulatedstring.setString("This a string that can't fail just because \"can't\" is in the string");

Choose a reason for hiding this comment

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

This test seems redundant with Test case #1. Can it be replaced with testing extra whitespace (EX: " abc") and the one where string has more than 1 whitespace between words (EX: "abc def")?

assertEquals(sStings[1], "string");
}

// test to make sure that it can scale to more substrings
Copy link

@Emc2Ikeda Emc2Ikeda Jun 14, 2023

Choose a reason for hiding this comment

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

Recommend modifying this test case so that it calls getSubStrings(1,15) (i.e. get all the words in a string).

}

// tests thats endWord cannot be <= 0 and should throw a
// IllegalArgumentException
Copy link

@Emc2Ikeda Emc2Ikeda Jun 14, 2023

Choose a reason for hiding this comment

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

Modify testGeSubStrings4 so that startWord is an integer greater than 0. This ensures that the exception is caused by the endWord <= 0, not because startWord is <= 0.

@@ -1,33 +1,94 @@
public class StringManipulation implements StringManipulationInterface {

Choose a reason for hiding this comment

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

Modify or add additional test cases in StringManipulationTest.java. If new test cases failed, modify this file so that it passes the said test cases.

@@ -2,7 +2,6 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Copy link

@Emc2Ikeda Emc2Ikeda Jun 14, 2023

Choose a reason for hiding this comment

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

Recommend adding some test cases for setString() and getString() for sake of sanity. Be sure to add cases when this.string is null and when you try to set this.string to null as well as when string is empty.

array = new int[] { 0, 1, 2, 3, 4, 5 };
assertThrows(IllegalArgumentException.class, () -> manipulatedstring.restoreString(array));
}

Choose a reason for hiding this comment

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

Add another test case for restoreString() for case when string contains more than 1 word, such as "Hello world".

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;


Choose a reason for hiding this comment

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

Also add cases in each method when string is null or empty.

manipulatedstring.setString("This Test should throw IndexOutOfBoundsException for endWord > string length");
assertThrows(IndexOutOfBoundsException.class, () -> manipulatedstring.getSubStrings(1, 100));
}

Choose a reason for hiding this comment

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

Add test case for getSubStrings() when startWord = endWord.

Copy link

@Emc2Ikeda Emc2Ikeda left a comment

Choose a reason for hiding this comment

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

Watch out for corner cases not described in the interface. For example, how should methods act when this.string is null or empty? I noted the ones I found in inline comments for StringManipulationTest.java. Then, re-test the StringManipulation class. If the new test cases fail, modify StringManipulation so that it passes all the new tests.

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