Skip to content

Add interface and junit implementation#4

Open
bpsix wants to merge 3 commits intomainfrom
Brady_Six_CodeReview
Open

Add interface and junit implementation#4
bpsix wants to merge 3 commits intomainfrom
Brady_Six_CodeReview

Conversation

@bpsix
Copy link

@bpsix bpsix commented Jun 15, 2023

No description provided.

@sarafarag sarafarag requested a review from IgorJanGit June 15, 2023 18:39
@IgorJanGit
Copy link

IgorJanGit commented Jun 17, 2023

Code is good, with tons of functions that I wish I had done in my code, as it is more understandable than my code. A minor issue is that comment is a bit too vague. It says what codes do but not with the context of why it does. A quick explanation of code would be good. I have the same issue with comments in my own code

Otherwise, get the number of words in string, which are
separated by one or more whitespace characters.
*/
return string.split("[\\s\\r\\t\\n]+").length;

Choose a reason for hiding this comment

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

you could for this just do String[] words = string.split("\s+"); it less characters and it does job of the rest of \r\t\n. here the definition https://learn.microsoft.com/en-us/dotnet/standard/base-types/character-classes-in-regular-expressions#whitespace-character-s

Comment on lines 52 to 62
public String[] getSubStrings(int startWord, int endWord){
//check null string
if(this.string == null) return null;
//check startWord <= 0
if(startWord <= 0) throw new IllegalArgumentException("startWord must be greater than zero");
//check endWord <= 0
if(endWord <= 0) throw new IllegalArgumentException("endWord must be greater than zero");
//check startWord > endWord
if(startWord > endWord) throw new IllegalArgumentException("startWord cannot be larger than endWord");
//check string.count() < endWord
if(this.count() < endWord) throw new IndexOutOfBoundsException("endWord cannot be larger than the total word count");

Choose a reason for hiding this comment

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

use to blank likes makes it easier to read the code also could combine the exception with or statements like this if (startWord <= 0 || endWord <= 0 || startWord > endWord) {} you could decrease the number of lines used

Comment on lines 69 to 82
//check null string
if(this.string == null) return null;
//check unequal lengths
if(this.string.length() != indices.length) throw new IllegalArgumentException("String length must match array length");
//check valid indices
for(int i = 0; i < indices.length; i++) {
if(indices[i] < 0 || indices[i] >= this.string.length()) throw new IndexOutOfBoundsException("Invalid array index");
}
//average case
StringBuilder b = new StringBuilder();
for(int i = 0; i < indices.length; i++) {
b.append(this.string.charAt(indices[i]));
}
return b.toString();

Choose a reason for hiding this comment

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

The minor issue about the comment you can expand to explain what is going on with context because comments are redundant with the code it is something I, too, struggle with in my code, the code looks good. i did it slightly differently where can put in IndexOutOfBoundsException in the for loop so it saves space.

public int count() {
return 0;
//If string is null or blank, return 0.
if(this.string == null || this.string.isBlank()) return 0;

Choose a reason for hiding this comment

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

This is great i should have done this in my code it fairly simple and works

Comment on lines +37 to +38
if(n == 1 && !maintainSpacing) return "";
if(n == 1 && maintainSpacing) return this.string.replaceAll(".", " ");

Choose a reason for hiding this comment

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

you could make this as an inbulit feature of the for loop to save space and handle edge case

assertEquals("I' b tt r ut s0 e 16 ts in th s tr n6 r gh ?", manipulatedstring.removeNthCharacter(3, true));
}

//Throws IndexOutOfBoundsException when n larger than string length

Choose a reason for hiding this comment

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

testing looks good but i would add for exception test for when it true just to double check if it works

manipulatedstring.setString("12345 6789");
String result = manipulatedstring.removeNthCharacter(1, true);
int length = result.length();
assertEquals(10, length);

Choose a reason for hiding this comment

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

did not think about doing this test in my own code looks good

@bpsix
Copy link
Author

bpsix commented Jun 20, 2023

Thanks for the feedback. I went back and added spacing between my comments and removed the redundancies from my regexes.

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