-
Notifications
You must be signed in to change notification settings - Fork 364
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
[Nicholas Arlin Halim] iP #276
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.
Coding standard wise it mostly look fine!
Some possible improvements are commented.
src/main/java/Duke.java
Outdated
public class Duke { | ||
private static Scanner sc; | ||
private static List<Task> 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.
Perhaps a better variable name than list here to indicate what the list holds?
private static List<Task> list; | |
private static List<Task> tasks; |
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,13 @@ | |||
public class Deadline extends Task { | |||
protected String by; |
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 a more descriptive variable name would be more helpful.
protected String by; | |
protected String dueDate; |
src/main/java/Duke.java
Outdated
} else if (isMark(input, list.size())) { | ||
int taskindex = Integer.parseInt(input.substring(5)) - 1; | ||
list.get(taskindex).markAsDone(); | ||
System.out.println("Solid! I'm marking this task as done:\n" + list.get(taskindex).toString()); |
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.
Line may have been too long, perhaps separate into multiple lines to be more aligned with the coding standard. There are also more similar issues further below.
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,13 @@ | |||
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.
I believe organising your Deadline, Todo, Event and task into a single package will help with the structure of your files. Also helps as you implement more classes.
src/main/java/Duke.java
Outdated
} | ||
} | ||
|
||
public static boolean isMark(String input, int listSize) { |
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.
All your methods are aptly named with the right names and format. Nice!
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.
LGTM, almost. Just a few nits to fix.
Overall, the code is well done 😄
src/main/java/Duke.java
Outdated
if ("bye".equals(input)) { | ||
break; | ||
} else if ("list".equals(input)) { | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < list.size(); i++) { | ||
int j = i + 1; | ||
System.out.println(j + "." + list.get(i).toString()); | ||
} | ||
} else if (isMark(input, list.size())) { | ||
int taskindex = Integer.parseInt(input.substring(5)) - 1; | ||
list.get(taskindex).markAsDone(); | ||
System.out.println("Solid! I'm marking this task as done:\n" + list.get(taskindex).toString()); | ||
} else if (isUnMark(input, list.size())) { | ||
int taskindex = Integer.parseInt(input.substring(7)) - 1; | ||
list.get(taskindex).markAsNotDone(); | ||
System.out.println("Aight, marking this as not done:\n" + list.get(taskindex).toString()); | ||
} else if (isDelete(input, list.size())) { | ||
int taskindex = Integer.parseInt(input.substring(7)) - 1; | ||
String removed = list.get(taskindex).toString(); | ||
list.remove(taskindex); | ||
System.out.println("Swee! One less task to go! Removing...\n" + removed); | ||
} else { //Task Creation | ||
Task task = null; | ||
if (isToDo(input)) { | ||
task = new ToDo(input); | ||
list.add(task); | ||
} else if (isDeadline(input)) { | ||
int index = input.indexOf(" /by "); | ||
task = new Deadline(input.substring(0, index), input.substring(index + 5)); | ||
list.add(task); | ||
} else if (isEvent(input)) { | ||
int fromdex = input.indexOf(" /from "); | ||
int todex = input.indexOf(" /to "); | ||
task = new Event(input.substring(0, fromdex), input.substring(fromdex + 7, todex), input.substring(todex + 5)); | ||
list.add(task); | ||
} else { | ||
throw new DukeException("What are you saying?\n" + "Please input a task with either todo, deadline or event prefixed!"); | ||
} | ||
System.out.println("Roger. This task has been added:\n" + " " + task.toString()); | ||
System.out.println("Now you have " + list.size() + " tasks in your 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.
Maybe you could use switch case statements instead of if-else statements for better readability? 🤔
src/main/java/Duke.java
Outdated
|
||
public static boolean isDeadline(String input) throws DukeException { | ||
if (input.length() >= 8 && input.startsWith("deadline")) { | ||
if (input.equals("deadline") || input.substring(8).isBlank() || input.equals("deadline /by") || input.equals("deadline /by ")) { |
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 would recommend breaking this statement into 2 lines to make the code look more neat.
src/main/java/Duke.java
Outdated
int fromdex = input.indexOf(" /from "); | ||
int todex = input.indexOf(" /to "); | ||
if (fromdex + 7 > todex) { | ||
throw new DukeException("What are you saying?"); | ||
} |
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.
There seems to be additional indentation to these 5 lines of code.
src/main/java/Task.java
Outdated
|
||
@Override | ||
public String toString() { | ||
return "[" + getStatusIcon() + "] " + getDescription(); |
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.
Is getDescription() necessary here? 🤔
return "[" + getStatusIcon() + "] " + getDescription(); | |
return "[" + getStatusIcon() + "] " + description; |
src/main/java/Duke.java
Outdated
public static boolean isUnMark(String input, int listSize) { | ||
if (input.length() >= 8 && input.startsWith("unmark ") && isNumeric(input.substring(7))) { | ||
int taskindex = Integer.parseInt(input.substring(7)) - 1; | ||
return !(taskindex < 0 || taskindex > listSize - 1); | ||
} | ||
return false; | ||
} | ||
|
||
public static boolean isDelete(String input, int listSize) { | ||
if (input.length() >= 8 && input.startsWith("delete ") && isNumeric(input.substring(7))) { | ||
int taskindex = Integer.parseInt(input.substring(7)) - 1; | ||
return !(taskindex < 0 || taskindex > listSize - 1); | ||
} | ||
return false; | ||
} | ||
|
||
public static boolean isToDo(String input) throws DukeException { | ||
if (input.length() >= 4 && input.startsWith("todo")) { | ||
if (input.equals("todo") || input.substring(4).isBlank()) { | ||
throw new DukeException("TODO needs a description!"); | ||
} else return input.startsWith("todo "); | ||
} | ||
return false; | ||
} |
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 you have defined your functions clearly and also applied the KISS rule. Well done 😄
Command classes are all user commands parsed from user inputs, and are executable. Extracting a Command class allows for better OOP and modularity of the code. Execution of commands is extracted from Parser class into Command classes. Each command is a subclass of the abstract Command class and each command implements execute() differently. This allows for modularity and better OOP so more commands can easily be added in the future. The code is also easier to understand and functionality and execution of the commands are abstracted away into the corresponding Command classes instead of just the Parser class.
Add assertions in Command, Task and Duke
Improve code quality
…ser and Gigachad icons
Update UG
DukeMeister3000
DukeMeister3000 tracks your tasks and gives you that edge to complete them on time. It is
quiteveryABSOLUTELY simple to useSimply
Let DukeMeister3000 manage your tasks for you! 💯
Some features:
If you are a Java programmer, you can review the code easily with its well designed code and JavaDocs. Here's the
newEvent
method: