Skip to content

Initial Commit#2

Open
Emc2Ikeda wants to merge 1 commit intoSWE-CS410:mainfrom
Emc2Ikeda:main
Open

Initial Commit#2
Emc2Ikeda wants to merge 1 commit intoSWE-CS410:mainfrom
Emc2Ikeda:main

Conversation

@Emc2Ikeda
Copy link

No description provided.

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

@haileymandal haileymandal left a comment

Choose a reason for hiding this comment

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

Your test cases look great. You did a few corner cases that I didn't think about. One suggestion I have for them is changing the test case methods to show what you are doing vs comments above them.

For example: "public void removeNthCharacter_maintainSpacingFalse_returnsStringWithoutSpaces"

This is formatting and up to developers preference but I found it helpful when doing it myself.



// Parameterized constructor
StringManipulation(String string) {

Choose a reason for hiding this comment

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

I see you have separate constructors, one default and other parameterized. I was thinking you could merge and have one constructor. What do you think?

throw new IllegalArgumentException("n has to be at least 1");
}

String string = this.getString();

Choose a reason for hiding this comment

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

I see in the last method, you called getString();. I think you could do the same here and do not need this.

this.string = string;
}

// todo: raise nullPointer Exception if string is null

Choose a reason for hiding this comment

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

I noticed this comment you had left for todo. I thought I would share that this is a great idea for a test case.

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