Khalil coats code review by KhalilxCoats · Pull Request #21 · SWE-CS410/5540-Spring23CodeReviewActivity

KhalilxCoats · 2023-06-15T07:51:51Z

No description provided.

silkycode

Good job with the code assignment! The majority of my comments are mostly in regards to small optimizations/readability improvements, so I don't consider them necessarily must-fix before approval. In the case of your unit testing, I believe that your tests largely cover the behavior cases that you expect from your methods, but I think that they could also be somewhat improved by refining their scope by splitting them up to test a single functionality per test.

-Nicholas C

silkycode · 2023-06-19T23:33:23Z

src/main/java/StringManipulation.java

-    @Override
+public class StringManipulation implements StringManipulationInterface {
+
+	private String objString;


Good use of private access modifier to ensure encapsulation of class String member

silkycode · 2023-06-19T23:37:51Z

src/main/java/StringManipulation.java

    public int count() {
-        return 0;
+
+    	String[] words = this.getString().split(" +");


Good use of one-or-many whitespaces to split on, but because split() uses regex, perhaps you could go even a step farther and use "\s+" instead of " +" so that all potential characters that separate words (like newlines, whitespace, etc.) are split upon.

silkycode · 2023-06-19T23:41:01Z

src/main/java/StringManipulation.java

+    			}
+    			this.setString(String.valueOf(objChars));
+    		}
+    		else if(!maintainSpacing)


Could use just a simple else here since maintainSpacing only ever has two possibilities

silkycode · 2023-06-19T23:46:51Z

src/main/java/StringManipulation.java

+    	else
+    	{
+    		returnList = new String[endWord-startWord+1];
+    		int loopStart = startWord;


Can discard and use value of startWord in the following loop where loopStart is, since startWord is passed as an argument originally

silkycode · 2023-06-19T23:48:26Z

src/main/java/StringManipulation.java

+    	{
+    		throw new IllegalArgumentException("upper bound greater than lower bound");
+    	}
+    	else if(endWord > this.count())


Since you've already done the work of splitting the String into words in line 66, you could compare the length of that array instead of needing to call the count() method for error checking, but good idea to use existing methods

silkycode · 2023-06-19T23:52:54Z

src/main/java/StringManipulation.java

-    public String restoreString(int[] indices) {
-        return null;
+    public String restoreString(int[] indices){
+    	String restore = "";


Really small optimization, but can throw the creation of the String to be returned after the error checking

silkycode · 2023-06-19T23:57:40Z

src/main/java/StringManipulation.java

+    		{
+    			newChars[i] = this.getString().charAt(indices[i]);
+    		}
+    		restore = (String.valueOf(newChars));


Using the String variable 'restore' is good for enhancing the readability of the code, but can also be left out since you could also return the 'newChars' array as a String with the same method you're using to assign it to 'restore'

silkycode · 2023-06-20T00:13:38Z

src/test/java/StringManipulationTest.java

+    	//blanks out string, maintainSpacing=false
+    	manipulatedstring.setString("the quick brown fox jumps over the lazy dog");
+        assertEquals("", manipulatedstring.removeNthCharacter(1, false));
    }


6 and 7 are good tests, it is worth testing to verify a behavior that you'd expect like this when n = 1.

silkycode · 2023-06-20T00:19:34Z

src/test/java/StringManipulationTest.java

+
+            assertEquals("Word position argument must be positive nonzero number.", thrown.getMessage());
    }



For getSubStrings() tests, because the behavior you're trying to verify is that the method throws an exception, the assertEquals statements may fall outside their scopes. The assertEquals() statements are best used in tests where you're testing the output, rather than the error checking.

TimSon1022

Overall the code looks well-structured and readable. There are some things you can change and edit but it's very small, so they are not necessarily prioritized changes. Great job though!

TimSon1022 · 2023-06-19T23:43:06Z

src/main/java/StringManipulation.java

+    		for(int i = 0; i < endWord- startWord+1; i++)
+    		{
+    			returnList[i] = words[loopStart - 1 + i];
+    			System.out.println(returnList[i]);


Make sure to remove any debugging System.out.println statements that are not needed in the final code

TimSon1022 · 2023-06-19T23:58:03Z

src/main/java/StringManipulation.java

+    	}
+    	else if(n > this.getString().length()-1)
+    	{
+    		throw new IndexOutOfBoundsException("index out of bounds");


The error message can be more descriptive here for if n is more than the string's length minus 1

TimSon1022 · 2023-06-20T00:10:17Z

src/main/java/StringManipulation.java

+    	}
+    	else if(endWord > this.count())
+    	{
+    		throw new IndexOutOfBoundsException("upper bound or lower is greater than word count");


In the error statement, you can add to the statement "or startWord is larger than the word count" to complete your error message.

TimSon1022 · 2023-06-20T01:29:20Z

src/main/java/StringManipulation.java

    public int count() {
-        return 0;
+
+    	String[] words = this.getString().split(" +");


you can use \s+ instead of + so it can represent all whitespace characters

KhalilxCoats added 2 commits June 15, 2023 00:40

Added final implementation of Stringmanipulation class.

0ca20e2

Updated StringManipulationTest.java with final testing implementation.

08b2ab0

KhalilxCoats closed this Jun 15, 2023

KhalilxCoats reopened this Jun 15, 2023

sarafarag requested review from TimSon1022 and silkycode June 15, 2023 16:34

silkycode approved these changes Jun 20, 2023

View reviewed changes

TimSon1022 reviewed Jun 20, 2023

View reviewed changes


		assertEquals("Word position argument must be positive nonzero number.", thrown.getMessage());
		}

Conversation

KhalilxCoats commented Jun 15, 2023

Uh oh!

silkycode left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silkycode Jun 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimSon1022 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

silkycode left a comment •

edited

Loading

silkycode Jun 19, 2023 •

edited

Loading