Saralyn's pull request for Code Review Activity#1
Conversation
seanfite
left a comment
seanfite
left a comment
There was a problem hiding this comment.
Good job, the main thing I would change is the fail("Not yet implemented") lines from the test cases
| @Test | ||
| public void testRemoveNthCharacter4() { | ||
| fail("Not yet implemented"); | ||
| manipulatedstring.setString("I'd b3tt3r put s0me d161ts in this 5tr1n6, right?"); |
There was a problem hiding this comment.
I would change to assertEquals("") and not have all the whitespace
|
I just realized that as well and am fixing now. Sorry and thank you for the heads up!
________________________________
From: Sara Farag ***@***.***>
Sent: Sunday, June 18, 2023 6:03 PM
To: SWE-CS410/5541-Spring23CodeReviewActivity ***@***.***>
Cc: Sean Fite ***@***.***>; Mention ***@***.***>
Subject: Re: [SWE-CS410/5541-Spring23CodeReviewActivity] Saralyn's pull request for Code Review Activity (PR #1)
CAUTION: This email originated from outside Bellevue College. Do not click links or open attachments unless you validate the sender and know the content is safe. If you are unsure, contact the Service Desk at x4357 (425-564-4357) or email ***@***.***
@sarafarag commented on this pull request.
________________________________
In src/main/java/StringManipulation.java<#1 (comment)>:
@OverRide
public String getString() {
- return null;
@seanfite<https://github.com/seanfite>
This is not the correct file you should review. This file shows the original skeleton code. The file highlighted in green is the code added by @SaralynOgden<https://github.com/SaralynOgden> for the implementation, which needs to be reviewed.
This is the same case for all your comments below. As you have given recommendations for changes to things already changed.
Please check the file changed tab and look for the code highlighted in green for review.
—
Reply to this email directly, view it on GitHub<#1 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/A7RN47ONVONMLBDFDXVLU4TXL6QM3ANCNFSM6AAAAAAZHECTJM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
| for (int i = 0; i < string.length(); i++) { | ||
| if ((i + 1) % n == 0) { | ||
| if (maintainSpacing) { | ||
| stringBuilder.append(" "); |
There was a problem hiding this comment.
I think changing .append to .deleteCharAt() could add some optimization by possibly avoiding creating a new string for each character removal
| public String restoreString(int[] indices) { | ||
| return null; | ||
| } | ||
| public String restoreString(int[] indices){ |
There was a problem hiding this comment.
Consider adding more descriptive object names to enhance readability
| @Test | ||
| public void testCount4() { | ||
| fail("Not yet implemented"); | ||
| manipulatedstring.setString( |
There was a problem hiding this comment.
For readability, it might be better to create this string via a random char generator in a loop so it doesn't fill up so much space.
seanfite
left a comment
seanfite
left a comment
There was a problem hiding this comment.
Firstly, so sorry for the first review. I had the wrong impression about the text lines highlighted red. Overall great code, some tiny optimization suggestions that could help but not necessarily required.
2 participants
No description provided.