Skip to content

Thomas/Paige Riffle Code Review Pull Request #33

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Trifflenator wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Thomas/Paige Riffle Code Review Pull Request #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 95 additions & 8 deletions src/main/java/StringManipulation.java
View file
Open in desktop
Copy link

@kemeng001 kemeng001 Jun 20, 2023

Choose a reason for hiding this comment

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

This implementation basically meets the performance and functional requirements, but still needs to be improved in some places to achieve better robustness.

Original file line number Diff line number Diff line change
@@ -1,32 +1,119 @@
public class StringManipulation implements StringManipulationInterface {

String str = null;
@Override
public String getString() {
return null;
return str;
}

@Override
public void setString(String string) {
str = string;
}

@Override
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 == " ") {
Copy link

@kemeng001 kemeng001 Jun 20, 2023

Choose a reason for hiding this comment

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

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;
Copy link

@kemeng001 kemeng001 Jun 20, 2023

Choose a reason for hiding this comment

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

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.

}
int n = 1;
for(int i = 1; i < str.length()-1; i++) {
//adds a word count at a space, but doesn't count up for multiple spaces in a row, or a space at the end of a string.
if(str.charAt(i) == ' ' && str.charAt(i-1) != ' ') {
n++;
}
}
return n;
}

@Override
public String removeNthCharacter(int n, boolean maintainSpacing) {
return null;
//exception check
String output = "";
if(n <= 0) {
throw new IllegalArgumentException("n is less than or equal to zero");
}
if( n > str.length()) {
throw new IndexOutOfBoundsException("n is greater than string length");
}
//iterates over string in increments of n, replacing those words, using substring function of String to grab the other snippets.
for(int i = n; i <= str.length(); i+=n) {
output += str.substring(i-n, i-1);
if(maintainSpacing) {
output += " ";
}
}
//Grabs the remaining portion of the string after the last removed character.
int remain = str.length()%n;
if(remain != 0) {
output += str.substring(str.length() - remain);
}
return output;
}

@Override
public String[] getSubStrings(int startWord, int endWord) {
return null;
public String[] getSubStrings(int startWord, int endWord){
Copy link

@spadeX6 spadeX6 Jun 20, 2023

Choose a reason for hiding this comment

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

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.

//Exceptions for invalid inputs
if(endWord > count() ) {
throw new IndexOutOfBoundsException("String has less words than endWord");
}

if(startWord <= 0) {
throw new IllegalArgumentException("startWord must be greater than zero");
}
if(endWord <= 0) {
throw new IllegalArgumentException("endWord must be greater than zero");
}
if(endWord < startWord) {
throw new IllegalArgumentException("endWord must not be less than startWord");
}
//Creates the string array, sets it to blank so there aren't any null issues.
String[] result = new String[endWord-startWord + 1];
for(int i = 0; i < result.length; i++) {
result[i] = "";
}
int words = 0;
int x = 0;
//Iterates over the whole string, only counts where words would be, copying it into the result
for(int i = 0; i < endWord; i++) {
while( x < str.length() && str.charAt(x) != ' ') {
if(i >= startWord-1) {
result[words] += str.charAt(x);
}
x++;
}
if(i >= startWord-1) {
words++;
}
x++;
}
return result;
}

@Override
public String restoreString(int[] indices) {
return null;
public String restoreString(int[] indices){
//Exception for incorrect int array
if(indices.length != str.length()) {
Copy link

@kemeng001 kemeng001 Jun 20, 2023

Choose a reason for hiding this comment

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

Check whether indices is null before using may make the code more robust, or when try to use indices.length may cause NullPointerException.

throw new IllegalArgumentException("Number of Indices must equal length of string");
}
//Create char array of same length of string.
char[] result = new char[str.length()];
int i = 0;
//Iterates over the indices and the string, assigning the string chars based on index values
for(int x : indices) {
//Exception handling for invalid indices.
if(x < 0) {
throw new IndexOutOfBoundsException("index must be above 0");
}
if( x >= str.length() ) {
throw new IndexOutOfBoundsException("Index must be under string length");
}
result[x] = str.charAt(i);
i++;
}
//Use str constructor to convert from char array.
String stringResult = new String(result);
return stringResult;
}


Expand Down
108 changes: 79 additions & 29 deletions src/test/java/StringManipulationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import org.junit.jupiter.api.AfterEach;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;

Expand All @@ -25,20 +26,26 @@ public void testCount1() {
int length = manipulatedstring.count();
assertEquals(4, length);
}

//Checks to see if default value for non-null/empty strings is 1
@Test
public void testCount2() {
fail("Not yet implemented");
manipulatedstring.setString("Hello");
int length = manipulatedstring.count();
assertEquals(1, length);

}

//Checks to ensure that it properly measures null strings as 0 words
@Test
public void testCount3() {
fail("Not yet implemented");
int length = manipulatedstring.count();
assertEquals(0, length);
}

//Checks to see if it's counting multiple spaces as multiple words.
@Test
public void testCount4() {
fail("Not yet implemented");
manipulatedstring.setString("This checks for long spaces");
int length = manipulatedstring.count();
assertEquals(5, length);
}

@Test
Expand All @@ -53,29 +60,38 @@ public void testRemoveNthCharacter2() {
assertEquals("I' b tt r ut s0 e 16 ts in th s tr n6 r gh ?", manipulatedstring.removeNthCharacter(3, true));
}

//checks to see if it throws exception when the index is larger than the number of chars
@Test
public void testRemoveNthCharacter3() {
fail("Not yet implemented");
}

manipulatedstring.setString("Hello");
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {manipulatedstring.removeNthCharacter(12, true); });

}
//Checks for exception when -1, an illegal argument, is passed.
@Test
public void testRemoveNthCharacter4() {
fail("Not yet implemented");
}
manipulatedstring.setString("Hello");
Assertions.assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.removeNthCharacter(-1, true); });

}
//Testing to see if it will properly remove all chars when value is 1.
@Test
public void testRemoveNthCharacter5() {
fail("Not yet implemented");
manipulatedstring.setString("Hello");
assertEquals(" ", manipulatedstring.removeNthCharacter(1, true));
}

//testing to see if it properly removes the last character of the string when the input number is a factor of the string length
@Test
public void testRemoveNthCharacter6() {
fail("Not yet implemented");
manipulatedstring.setString("Ten Chars!");
assertEquals("Ten hars ", manipulatedstring.removeNthCharacter(5,true));
}

//Testing to check whole string iteration, like test 5 without maintaining spacing.
@Test
public void testRemoveNthCharacter7() {
fail("Not yet implemented");
manipulatedstring.setString("Hello");
assertEquals("", manipulatedstring.removeNthCharacter(1, false));
}

@Test
Expand All @@ -86,26 +102,50 @@ public void testGeSubStrings1() {
assertEquals(sStings[0], "my");
assertEquals(sStings[1], "string");
}

//Checks for index out of bound exception being thrown
@Test
public void testGeSubStrings2() {
fail("Not yet implemented");
manipulatedstring.setString("This is my string");
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {manipulatedstring.getSubStrings(1, 5);});
}
//checks for startWord being <= 0 throwing an exception
@Test
public void testGeSubStrings3() {
fail("Not yet implemented");
manipulatedstring.setString("test with many words for exceptions");
Exception exception = assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.getSubStrings(0, 1);});
String expected = "startWord must be greater than zero";
String actual = exception.getMessage();
assertTrue(actual == expected);
Copy link

@kemeng001 kemeng001 Jun 20, 2023

Choose a reason for hiding this comment

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

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.

}
//checks for endWord being <= 0 throwing an exception
@Test
public void testGeSubStrings4() {
fail("Not yet implemented");
manipulatedstring.setString("test with many words for exceptions");
Exception exception = assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.getSubStrings(1, 0);});
String expected = "endWord must be greater than zero";
String actual = exception.getMessage();
assertTrue(actual == expected);
}
//Checks for endWord being less than startWord throwing an exception
@Test
public void testGeSubStrings5() {
fail("Not yet implemented");
manipulatedstring.setString("test with many words for exceptions");
Exception exception = assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.getSubStrings(4, 2);});
String expected = "endWord must not be less than startWord";
String actual = exception.getMessage();
assertTrue(actual == expected);
}
//Checks to see how well it works on a string with many words.
@Test
public void testGeSubStrings6() {
fail("Not yet implemented");
manipulatedstring.setString("This is a long string");
String [] sStrings = manipulatedstring.getSubStrings(1, 5);

assertEquals(sStrings[0], "This");
assertEquals(sStrings[1], "is");
assertEquals(sStrings[2], "a");
assertEquals(sStrings[3], "long");
assertEquals(sStrings[4], "string");
}

@Test
Expand All @@ -118,31 +158,41 @@ public void testRestoreString1()
assertEquals(restoreString, "rat");
}

//checks to see if it throws an exception when an array that is shorter than the string size is passed
@Test
public void testRestoreString2()
{
fail("Not yet implemented");
manipulatedstring.setString("short");
int[] array = {1, 2, 3};
Assertions.assertThrows(IllegalArgumentException.class, () -> {manipulatedstring.restoreString(array);});

}

//checks to see if an exception is thrown when an index over the string length is passed
@Test
public void testRestoreString3()
{
fail("Not yet implemented");
manipulatedstring.setString("short");
int[] array = {1, 2, 3, 4, 5};
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {manipulatedstring.restoreString(array);});

}

//checks to see if exception is thrown when index below 0 is thrown.
@Test
public void testRestoreString4()
{
fail("Not yet implemented");

manipulatedstring.setString("short");
int[] array = {1, 2, 3, 4, -1};
Assertions.assertThrows(IndexOutOfBoundsException.class, () -> {manipulatedstring.restoreString(array);});
}

//test for longer, more complex usage of restore string, to check if method works (actually did catch that it wasn't quite right, so it worked.)
@Test
public void testRestoreString5()
{
fail("Not yet implemented");
manipulatedstring.setString("ixemdpusllyiingstr ");
int [] array;
array=new int[]{1, 2, 3, 0, 4, 7, 6, 9, 11, 12, 13, 10, 18, 19, 20, 15, 16, 17, 5, 8, 14};
String restoreString = manipulatedstring.restoreString(array);
assertEquals(restoreString, "mixed up silly string");

}

Expand Down