-
Notifications
You must be signed in to change notification settings - Fork 77
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
[Javier Ong Ding Quan] IP #64
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.
Very nicely organised :)
src/main/java/TaskList.java
Outdated
@@ -0,0 +1,57 @@ | |||
public class TaskList { | |||
Task[] list = new Task[100]; |
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.
Naming needs to be more specific
src/main/java/Duke.java
Outdated
} else if (input.contains("unmark")) { | ||
list.unmarkStatus(input); | ||
} else if (input.contains("mark")) { | ||
list.markStatus(input); |
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 constants rather than magic strings
src/main/java/Duke.java
Outdated
@@ -1,39 +1,25 @@ | |||
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.
Good that you only imported the library that you need!
src/main/java/TaskList.java
Outdated
System.out.println("Bye. Hope to see you again soon!"); | ||
printLine(); | ||
} | ||
public void addTask(String input) { |
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 that you followed the naming convention :)
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 the code is understandable, keep it up! Some issues need to be resolved to make your code more readable.
src/main/java/TaskList.java
Outdated
int number = Integer.parseInt(instructions[1]); | ||
list[number-1].isDone = 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.
Can be more specific, perhaps could use index instead of number
src/main/java/TaskList.java
Outdated
public void markStatus(String input) { | ||
String[] instructions = input.split(" "); | ||
int number = Integer.parseInt(instructions[1]); | ||
list[number-1].isDone = true; | ||
printLine(); | ||
System.out.println("Nice! I've marked this task as done:"); | ||
System.out.println(" [X] " + list[number-1].description); | ||
printLine(); | ||
} |
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.
Could be better if markStatus and unmarkStatus came from a more generic function. Both functions are very similar, hence there should be less repeat code
src/main/java/Task.java
Outdated
} | ||
|
||
public String getStatusIcon() { | ||
return (isDone ? "X" : " "); // mark done task with X |
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.
Try to minimize the usage lambda functions if possible as they make code less readable
src/main/java/TaskList.java
Outdated
Task t = new Task(input); | ||
list[index] = t; |
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.
Declaration of task can be more specific
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.
Generally, the code is quite ok.
There are some specific things I have mentioned in the comments that help to improve readability/maintainability of the code. Please check out.
src/main/java/Duke.java
Outdated
@@ -1,10 +1,83 @@ | |||
import consoleCommands.ConsoleCommands; | |||
import exception.*; |
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.
Please import classes explicitly
src/main/java/Duke.java
Outdated
} else { | ||
try { | ||
ConsoleCommands.addTask(input, taskList); | ||
} catch (InvalidArgumentsException e) { | ||
System.out.println(INVALID_ARGUMENTS_ERROR_MESSAGE); | ||
ConsoleCommands.printLine(); | ||
} catch (InvalidCommandException e) { | ||
System.out.println(COMMAND_DOES_NOT_EXIST_ERROR_MESSAGE); | ||
ConsoleCommands.printLine(); | ||
} catch (NotEnoughArgumentsException e) { | ||
System.out.println(NOT_ENOUGH_ARGUMENTS_ERROR_MESSAGE); | ||
ConsoleCommands.printLine(); | ||
} |
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.
Oh! no no... this is not very good to default the behavior for a single command.
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.
Hi Prof! I'm not quite sure what it means to "default the behavior for a single command", could you kindly clarify? Thank you!
public static final String BYE_MESSAGE = "Bye. Hope to see you again soon!"; | ||
public static void printLine() { | ||
System.out.println("____________________________________________________________"); | ||
} | ||
public static void markStatus(String input, ArrayList<Task> Array) throws TaskDoesNotExistException { | ||
printLine(); | ||
String[] instructions = input.split(" "); |
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.
Please improve spacing. Use the IDE to do it for you!
System.out.println(" " + Array.get(index-1)); | ||
printLine(); | ||
} | ||
public static void printList(ArrayList<Task> Array) { |
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.
Parameter naming is off convention/standards.
If it is a constant it should be ARRAY
, if not it should be array
.
Also, a better name would be useful.
System.out.println("Now you have " + Array.size() + " tasks in the list."); | ||
printLine(); | ||
} | ||
public static void addTask(String input, ArrayList<Task> Array) throws InvalidCommandException, InvalidArgumentsException, NotEnoughArgumentsException { |
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 seems particularly long. try to wrap it
} | ||
else if (instructions[0].equals(COMMAND_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.
coding standard violation here.
public static final String TRUE = "1"; | ||
public static final String FALSE = "0"; |
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.
These variable names could trip the reader/maintainer, given that there is already keywords of the same name
src/main/java/task/Deadline.java
Outdated
package task; | ||
|
||
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.
Why is this protected?
Are you inheriting anything else from deadline to make use of this field?
src/main/java/task/Task.java
Outdated
public class Task { | ||
protected String description; | ||
public boolean isDone; | ||
public Task(String description) { //creating new object |
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.
Errrm... that is what a constructor is for.
Please remove that redundant comment.
Update Duke.java
Edits done to code style after checkstyle Added comments for storage > readFromFile Edited UG
- Fixed bugs with date in event and deadline - Fixed bugs with find command - Fixed bugs with list command - Minor changes to naming conventions and documentation - Updated UG to fit new changes - Added new exception DateParseException
Levels-1,2,3 completed so far, Level-4 wip.