-
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
[Dyah Ayu Nurun Nafisah] iP #200
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.
Overall, good job in following the iP requirements and coding standards. There are a few areas of improvement that I pointed out for you to enhance your code. Keep up the good work!
src/main/java/duke/Duke.java
Outdated
} | ||
} | ||
|
||
public static void exitProgram() { |
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.
Consider using consistent method names across similar methods. For example, since you named your welcome message method showWelcomeMessage(), your exit message method is recommended to be showExitMessage() so that it is more consistent and easier for other people to trace through your code.
src/main/java/duke/Duke.java
Outdated
@@ -0,0 +1,184 @@ | |||
package duke; | |||
|
|||
import duke.exceptions.*; |
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.
You should import only the libraries that you are going to use explicitly instead of importing everything using the * wildcard. This makes your import libraries easier to comprehend and maintain.
src/main/java/duke/Duke.java
Outdated
private static final String COMMAND_EXIT_WORD = "bye"; | ||
private static final String COMMAND_LIST_WORD = "list"; | ||
private static final String COMMAND_DONE_WORD = "done"; | ||
private static final String COMMAND_TODO_WORD = "todo"; | ||
private static final String COMMAND_DEADLINE_WORD = "deadline"; | ||
private static final String COMMAND_EVENT_WORD = "event"; |
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.
Would it be better if you name your constants with the command words grouped at the front? For example, COMMAND_WORD_EXIT
.
src/main/java/duke/Duke.java
Outdated
while (!inputLine.equals(COMMAND_EXIT_WORD)) { | ||
try { | ||
taskCount = executeCommand(tasksList, taskCount, inputLine.trim()); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (IllegalCommand e) { | ||
System.out.println(" OOPS! I'm sorry, but I don't know what that means :("); | ||
System.out.println(" I can only execute these commands: "); | ||
for (int i = 1; i <= COMMAND_WORDS_LIST.length; i++) { | ||
System.out.println(" " + i + ". " + COMMAND_WORDS_LIST[i - 1]); | ||
} | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (EmptyToDoDescription e) { | ||
System.out.println(" OOPS! The description of a todo cannot be empty!"); | ||
System.out.println(" Please input again with the correct format!"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (IllegalDeadlineInput e) { | ||
System.out.println(" OOPS! Please input again using this format:!"); | ||
System.out.println(" 'deadline <description> /by <due date>'"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (IllegalEventInput e) { | ||
System.out.println(" OOPS! Please input again using this format:!"); | ||
System.out.println(" 'event <description> /at <event date>'"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (EmptyTaskNumber e) { | ||
System.out.println(" OOPS! Please specify the completed task number!"); | ||
System.out.println(" Example: 'done 1'"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (NumberFormatException e) { | ||
System.out.println(" OOPS! Please put only integer as the task number!"); | ||
System.out.println(" Example: 'done 1'"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} catch (NullPointerException e) { | ||
System.out.println(" OOPS! You must have typed the wrong number. Please type in again correctly!"); | ||
inputLine = INPUT_COMMAND.nextLine(); | ||
} |
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.
Since you are going to use the line inputLine = INPUT_COMMAND.nextLine()
no matter there is an exception caught or not, you could consider putting a finally
statement at the end and place the code there. With this, you could avoid repeating the same code for all exceptions.
src/main/java/duke/Duke.java
Outdated
private static final Scanner INPUT_COMMAND = new Scanner(System.in); | ||
|
||
public static void main(String[] args) { | ||
Task[] tasksList = new Task[MAX_TASKS_NUMBER]; |
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 job removing the magic literals!
src/main/java/duke/Duke.java
Outdated
} | ||
} | ||
|
||
public static int executeCommand(Task[] tasksList, int taskCount, String inputLine) throws IllegalCommand, EmptyToDoDescription, IllegalDeadlineInput, IllegalEventInput, EmptyTaskNumber { |
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 line length should not be more than 120 characters because it will be harder for other programmers to look at the code. You should wrap a long line to the next line for better readability.
return isDone; | ||
} | ||
|
||
public void setDone(boolean done) { |
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.
Should the name of the boolean done
be a boolean name instead?
super(description); | ||
} | ||
|
||
@Override |
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 the override statement for each child task class!
No description provided.