-
Notifications
You must be signed in to change notification settings - Fork 193
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
[Vincent Lau Han Leong] iP #183
base: master
Are you sure you want to change the base?
Conversation
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.
The codes are of precision with accurate format of coding as well as appropriate naming.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,15 @@ | |||
public class Deadline extends Task{ |
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.
A minor code improvement: add a space before the "{"
src/main/java/Duke.java
Outdated
case TODO: | ||
parsedOutput = new String[]{input}; | ||
break; | ||
|
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.
Perhaps you could delete the empty line between each case in the switch function. It was not shown in the lectures. Also, I think it is better to keep the consistency of format (since you did not add any empty line before or after this).
src/main/java/Duke.java
Outdated
if(input.length() != 6) { | ||
printIncorrectInputMessage(); | ||
return; | ||
} |
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.
Good use of this guard clause and I am able to see the "good path" of the method.
src/main/java/Duke.java
Outdated
switch(type) { | ||
case TODO: | ||
parsedOutput = new String[]{input}; | ||
break; | ||
|
||
case DEADLINE: | ||
parsedOutput = input.split(DEADLINE_DESCRIPTION_AND_DATE_SPLITTER); | ||
break; | ||
|
||
case EVENT: | ||
parsedOutput = input.split(EVENT_DESCRIPTION_AND_DATE_TIME_SPLITTER); | ||
break; | ||
} |
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.
Perhaps you could add a default
branch? The default
branch can be used to raise a suitable error as I think the execution should not reach the default
branch. I also noticed this same issue in your other switch
statements.
src/main/java/Duke.java
Outdated
} | ||
|
||
private static void processInput(String input) { | ||
switch(input.split(" ")[0].toLowerCase()) { |
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.
Perhaps you could create a variable with a meaningful name to simplify the expression in the switch statement?
src/main/java/Duke.java
Outdated
switch(input.split(" ")[0].toLowerCase()) { | ||
case "list" : | ||
executeListCase(); | ||
break; | ||
case "done" : | ||
executeDoneCase(input); | ||
break; | ||
case "todo" : | ||
executeTaskCase(input, TODO_STARTING_INDEX,TaskType.TODO); | ||
break; | ||
case "deadline" : | ||
executeTaskCase(input, DEADLINE_STARTING_INDEX,TaskType.DEADLINE); | ||
break; | ||
case "event" : | ||
executeTaskCase(input, EVENT_STARTING_INDEX,TaskType.EVENT); | ||
break; | ||
default : | ||
printIncorrectInputMessage(); |
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 like how the Single Level of Abstraction Principle is being applied here and I find it very easy to read and understand what you are trying to do.
src/main/java/duke/Duke.java
Outdated
case "todo": | ||
try { | ||
executeTaskCase(input, TODO_STARTING_INDEX, TaskType.TODO); | ||
|
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.
Suggest to keep consistent the new line convention throughout the code.
src/main/java/duke/Duke.java
Outdated
return parsedOutput; | ||
} | ||
|
||
private static void addTask(String input,TaskType type) throws StringIndexOutOfBoundsException, DukeException { |
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.
Suggest to relocate the function definition from "throw" onwards to a new line with appropriate line indent.
src/main/java/duke/Duke.java
Outdated
tasks[Task.getTotalTasks()] = new ToDo(parsedOutput[0]); | ||
break; | ||
case EVENT: | ||
if(parsedOutput.length < 2) { |
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.
Suggest to replace magic number with a well-named constant.
src/main/java/duke/Duke.java
Outdated
printWordsWithIndentation(LIST_IS_EMPTY); | ||
} | ||
for (int i = 0; i < Task.getTotalTasks(); i++) { | ||
printWordsWithIndentation(i + 1 + "." + tasks[i].getStatusIconAndDescription()); |
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.
Suggest to relocate expression in function argument to a separate line with appropriate comments.
src/main/java/duke/Duke.java
Outdated
if(input.length() < 6) { | ||
throw new DukeException(); | ||
} | ||
int index = Integer.parseInt(input.split(" ")[1]) - 1; |
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.
Suggest to replace magic number with a well named constant.
# Conflicts: # src/main/java/duke/Duke.java # src/main/java/task/Task.java
Add date and time for event and deadline and some constants
# Conflicts: # src/main/java/commands/PrintOptions.java # src/main/java/parser/Parser.java # src/main/java/tasklist/TaskList.java # src/main/java/ui/Ui.java
Branch level 9
# Conflicts: # src/main/java/commands/AddCommand.java # src/main/java/commands/DeleteCommand.java # src/main/java/commands/DoneCommand.java # src/main/java/commands/IncorrectCommand.java # src/main/java/commands/ListCommand.java # src/main/java/duke/Duke.java # src/main/java/parser/Parser.java # src/main/java/task/Deadline.java # src/main/java/task/Event.java
Branch A-JavaDoc
No description provided.