[Cao] iP#466
Open
Ringo1225 wants to merge 70 commits into
Open
Conversation
added 10 commits
August 20, 2020 22:11
Allowed task with deadlines and date/time. Achieved level-4
Lingy12
reviewed
Aug 31, 2020
Lingy12
left a comment
Lingy12
left a comment
There was a problem hiding this comment.
There is no violation of the coding standard. You follow the naming layout and comment. But could you try to pack the statement, especially for the output (eg. pack printing output format into a function, so that you can just call that function to print output, instead of using multiple lines to print the lines etc). Also, perhaps adding some comment in each logical block and some Javadoc can provide better documentation.
yuanxing-z
reviewed
Aug 31, 2020
| package main.java; | ||
|
|
||
| public class Deadline extends Task { | ||
| protected String deadline; |
There was a problem hiding this comment.
Suggest using private keyword instead of protected.
yuanxing-z
reviewed
Aug 31, 2020
| + "|____/ \\__,_|_|\\_\\___|\n"; | ||
| System.out.println("Hello from\n" + logo); | ||
| System.out.println(" ____________________________________________________________"); | ||
| System.out.println(" Hello! I'm Duke\n What can I do for you?"); |
There was a problem hiding this comment.
different parts of the code can be grouped together as a method, make it easier for abstraction.
yuanxing-z
reviewed
Aug 31, 2020
| package main.java; | ||
|
|
||
| public class Event extends Task { | ||
| protected String time; |
yuanxing-z
reviewed
Aug 31, 2020
yuanxing-z
left a comment
yuanxing-z
left a comment
There was a problem hiding this comment.
Some suggestions are made as single comment in each file, overall the code quality is good. Feel free to reply if you have any questions regarding the comments.
added 6 commits
September 2, 2020 23:44
junhui-phoon
reviewed
Sep 6, 2020
| while (true) { | ||
| String command = sc.next(); | ||
| switch (command) { | ||
| case "bye": |
There was a problem hiding this comment.
There's no indention for the case statements. It should be 'aligned' with the switch statement.
junhui-phoon
reviewed
Sep 6, 2020
| try { | ||
| handleDone(); | ||
| } catch (DukeException e) { | ||
| System.out.println(" ____________________________________________________________"); |
There was a problem hiding this comment.
Maybe you could abstract out this border line as a variable, then call it whenever you need it. Makes the code tidy.
junhui-phoon
reviewed
Sep 6, 2020
Comment on lines
+99
to
+123
| public void handleDone() throws DukeException { | ||
| String doneCommand = sc.nextLine(); | ||
| int index = 0; | ||
| if (doneCommand.isEmpty()) { | ||
| throw new DukeException("\u2639 OOPS!!! I need to know the index of the task to be done!"); | ||
| } | ||
| try { | ||
| index = Integer.parseInt(doneCommand.stripLeading()); | ||
| } catch (NumberFormatException e) { | ||
| System.out.println(" ____________________________________________________________"); | ||
| System.out.println(" \u2639 Please enter a valid integer!!"); | ||
| System.out.println(" ____________________________________________________________\n"); | ||
| return; | ||
| } | ||
| if (index > taskList.size()) { | ||
| throw new DukeException("\u2639 Your number is too large!!"); | ||
| } | ||
| Task currentTask = taskList.get(index - 1); | ||
| currentTask.markAsDone(); | ||
| System.out.println(" ____________________________________________________________"); | ||
| System.out.println(" Nice! I've marked this task as done:"); | ||
| System.out.print(" "); | ||
| currentTask.printDescription(); | ||
| System.out.println(" ____________________________________________________________\n"); | ||
| } |
There was a problem hiding this comment.
Perhaps you could add appropriate line breaks to enhance readability
junhui-phoon
reviewed
Sep 6, 2020
Comment on lines
+126
to
+149
| String deleteCommand = sc.nextLine(); | ||
| int index = 0; | ||
| if (deleteCommand.isEmpty()) { | ||
| throw new DukeException("\u2639 OOPS!!! I need to know the index of the task to be deleted!"); | ||
| } | ||
| try { | ||
| index = Integer.parseInt(deleteCommand.stripLeading()); | ||
| } catch (NumberFormatException e) { | ||
| System.out.println(" ____________________________________________________________"); | ||
| System.out.println(" \u2639 Please enter a valid integer!!"); | ||
| System.out.println(" ____________________________________________________________\n"); | ||
| return; | ||
| } | ||
| if (index > taskList.size()) { | ||
| throw new DukeException("\u2639 Your number is too large!!"); | ||
| } | ||
| Task currentTask = taskList.get(index - 1); | ||
| taskList.remove(index - 1); | ||
| System.out.println(" ____________________________________________________________"); | ||
| System.out.println(" Noted! I've removed this task:"); | ||
| System.out.print(" "); | ||
| currentTask.printDescription(); | ||
| System.out.println(" Now you have " + taskList.size() + " tasks in the list."); | ||
| System.out.println(" ____________________________________________________________\n"); |
There was a problem hiding this comment.
Same comment as above. Line breaks for readability.
added 5 commits
September 7, 2020 22:57
* branch-Level-7: solved the bug when the command.txt does not exist. Saved the list of tasks in a txt. file. Every time will load the data from the file.
* branch-Level-8: Stored Deadline data as java.time.LocalDate # Conflicts: # src/main/java/CommandHandler.java # src/main/java/Deadline.java
* branch-GUI: Add FXML Restructure project Finish GUI feature Improve GUI Add some GUI feature Edit build.gradle Revise project structure
Add assertions
* 'master' of https://github.com/Ringo1225/ip: Add assertions
* master: Add assertions
* branch-A-CodeQuality: Improve code quality Add assertions
Improve code quality
* 'master' of https://github.com/Ringo1225/ip: Improve code quality
Add feature: FixedDurationTasks
* 'master' of https://github.com/Ringo1225/ip: Add feature: FixedDurationTasks
* 'master' of https://github.com/Ringo1225/ip: Set theme jekyll-theme-minimal
* 'master' of https://github.com/Ringo1225/ip: Set theme jekyll-theme-midnight
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.