-
Notifications
You must be signed in to change notification settings - Fork 426
[Lee Miao En Emelyn] iP #442
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.
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
base: master
Are you sure you want to change the base?
[Lee Miao En Emelyn] iP #442
Conversation
…ate BROWNIE responses
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1st review
LGTM 👍🏻 Standard mostly followed, just need to clean up a few issues.
| @@ -0,0 +1,102 @@ | |||
| import java.util.Scanner; | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of the public methods and classes are missing a Javadoc comment. Perhaps you can add comments to at least the classes?
| @@ -0,0 +1,102 @@ | |||
| import java.util.Scanner; | |||
|
|
|||
| public class Brownie{ | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a space before the brackets here?
| throw new DukeException("Hello! Can I confirm what task you would like to note down?\n" + | ||
| "ToDo tasks must look like: todo xxxxxxxx"); | ||
| } | ||
| tasks.add(taskDescription.substring(5), 'T'); | ||
| System.out.println(horizontalLine); | ||
| } else if (taskDescription.startsWith("event")) { | ||
| if (scriptLength < 7) { | ||
| throw new DukeException("Hello! Can I confirm what task you would like to note down?\n" + | ||
| "Event tasks must look like: event xxxxxxxx /from xxx /to xxx"); | ||
| } | ||
| tasks.add(taskDescription.substring(6), 'E'); | ||
| System.out.println(horizontalLine); | ||
| } else if (taskDescription.startsWith("deadline")) { | ||
| if (scriptLength < 10) { | ||
| throw new DukeException("Hello! Can I confirm what task you would like to note down?\n" + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you push the + down to the following line?
| public void deleteTask(int taskNumber) { | ||
| Task task = tasks.get(taskNumber); | ||
| tasks.remove(taskNumber - 1); | ||
| System.out.println("Noted. I've removed this task:\n" + task + "\nNow you have " + tasks.size() + " tasks in the list."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line is just a littttle bit too long. Can you split it into two?
| import java.util.Scanner; | ||
|
|
||
| public class Brownie{ | ||
| private static final String horizontalLine = "____________________________________________________________"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a constant, right? So I think the name should be HORIZONTAL_LINE?
| String greeting = "\nHello! I'm BROWNIE\nWhat can I do for you?\n"; | ||
| String farewell = "Bye. Hope to see you again soon!\n"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And these strings are constants, too?
No description provided.