-
Notifications
You must be signed in to change notification settings - Fork 362
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
[randallnhr] iP #389
base: master
Are you sure you want to change the base?
[randallnhr] iP #389
Conversation
src/main/java/Task.java
Outdated
protected String startDate; | ||
protected String endDate; | ||
|
||
public Task(String name, String date1, String date2) { |
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 to abstract out startDate and endDate to Event class?
src/main/java/Duke.java
Outdated
@@ -12,8 +13,9 @@ public static void main(String[] args) throws IOException { | |||
|
|||
BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); | |||
|
|||
String[] word = br.readLine().split(" "); | |||
String[] word = br.readLine().split(" ",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.
i like that you store word as a String array for easier manipulation
src/main/java/Duke.java
Outdated
@@ -12,8 +13,9 @@ public static void main(String[] args) throws IOException { | |||
|
|||
BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); | |||
|
|||
String[] word = br.readLine().split(" "); | |||
String[] word = br.readLine().split(" ",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.
would renaming word make it less vague and reflect more as a 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.
Overall, LGTM. Very clean code and very nice use of ternary operators. Nice abstraction and use of Inheritance to enforce the OOP principles. Good job!
src/main/java/Deadline.java
Outdated
public String toString() { | ||
return this.isDone | ||
? "[D][X] " + this.name + " (by: " + getDate(this.deadline) + ")" | ||
: "[D][ ] " + this.name + " (by: " + getDate(this.deadline) + ")"; | ||
} | ||
} |
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, instead of leaving the isDone logic in this toString() function, you could leave it in the Task class instead so that you dont have to retype so much code if you need to create a new Task type in the future.
src/main/java/Duke.java
Outdated
// try { | ||
// if (word[0].equals("list")) { | ||
// int curr = 1; | ||
// Iterator<Task> iter = lst.iterator(); | ||
// while (iter.hasNext()) { | ||
// System.out.println(curr + " " + iter.next()); | ||
// curr++; | ||
// } | ||
// word = br.readLine().strip().split(" ",2); | ||
// } else if (word[0].equals("mark")) { | ||
// if (word.length == 1) { | ||
// throw new DukeException("Mark needs a number."); | ||
// } | ||
// if (Integer.parseInt(word[1]) > count) { | ||
// throw new DukeException("Invalid task."); | ||
// } | ||
// Task t = lst.get(Integer.parseInt(word[1]) - 1); | ||
// t.isDone = true; | ||
// System.out.println("Task has been marked as done:\n " + t); | ||
// word = br.readLine().split(" ",2); | ||
// } else if (word[0].equals("unmark")) { | ||
// if (word.length == 1) { | ||
// throw new DukeException("Unmark needs a number."); | ||
// } | ||
// if (Integer.parseInt(word[1]) > count) { | ||
// throw new DukeException("Invalid task."); | ||
// } | ||
// Task t = lst.get(Integer.parseInt(word[1]) - 1); | ||
// t.isDone = false; | ||
// System.out.println("Task has been marked as not done:\n " + t); | ||
// word = br.readLine().split(" ",2); | ||
// } else if (word[0].equals("todo")) { | ||
// if (word.length == 1) { | ||
// throw new DukeException("todo needs a description"); | ||
// } | ||
// Task t = new Todo(word[1].strip()); | ||
// lst.add(t); | ||
// count++; | ||
// System.out.println("Added new todo:\n " + t + "\nNumber of tasks: " + count); | ||
// word = br.readLine().strip().split(" ",2); | ||
// } else if (word[0].equals("deadline")) { | ||
// if (word.length == 1 || !word[1].contains("/by")) { | ||
// throw new DukeException("Deadline needs a /by."); | ||
// } | ||
// String[] tempWord = word[1].strip().split("/by "); | ||
// if (tempWord.length == 1) { | ||
// throw new DukeException("/by needs a date/time."); | ||
// } | ||
// try { | ||
// Task t = new Deadline(tempWord[0].strip(), tempWord[1].strip()); | ||
// lst.add(t); | ||
// count++; | ||
// System.out.println("Added new deadline:\n " + t + "\nNumber of tasks: " + count); | ||
// word = br.readLine().strip().split(" ",2); | ||
// } catch (DateTimeParseException e) { | ||
// throw new DukeException("Date after /by needs to be in format yyyy-mm-dd"); | ||
// } | ||
// } else if (word[0].equals("event")) { | ||
// if (word.length == 1 || !word[1].contains("/from") || !word[1].contains("/to") ) { | ||
// throw new DukeException("Event needs a /from and /to."); | ||
// } | ||
// String[] tempWord = word[1].split("/"); | ||
// String[] from = tempWord[1].split(" ",2); | ||
// String[] to = tempWord[2].split(" ",2); | ||
// if (from.length == 1 || to.length == 1) { | ||
// throw new DukeException("/from and /to needs a date/time."); | ||
// } | ||
// try { | ||
// Task t = new Event(tempWord[0].strip(), from[1].strip(), to[1].strip()); | ||
// lst.add(t); | ||
// count++; | ||
// System.out.println("Added new event:\n " + t + "\nNumber of tasks: " + count); | ||
// word = br.readLine().strip().split(" ",2); | ||
// } catch (DateTimeParseException e) { | ||
// throw new DukeException("Date after /from and /to needs to be in format yyyy-mm-dd"); | ||
// } | ||
// } else if (word[0].equals("delete")) { | ||
// if (word.length == 1) { | ||
// throw new DukeException("Delete needs a number."); | ||
// } | ||
// if (Integer.parseInt(word[1]) > count) { | ||
// throw new DukeException("Invalid task."); | ||
// } | ||
// Task t = lst.remove(Integer.parseInt(word[1]) - 1); | ||
// count--; | ||
// System.out.println("Deleted task:\n " + t + "\nNumber of tasks: " + count); | ||
// word = br.readLine().strip().split(" ",2); | ||
// } else { | ||
// throw new DukeException("Sorry I do not understand the command"); | ||
// } | ||
// } catch (DukeException e) { | ||
// System.out.println(e.getMessage()); | ||
// word = br.readLine().strip().split(" ",2); | ||
// } | ||
// } | ||
// storeData(lst); | ||
// System.out.println("Duke: Goodbye"); | ||
// } catch (IOException e) { | ||
// System.out.println(e.getMessage()); | ||
// } |
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 suggest you to just delete all these unnecessary commented code. That is mainly the point of a version control system like git which is to undo any changes that you might not like in the future. Commenting them out and leaving them in code makes your code unnecssarily long and confusing.
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, I found your code easy to follow, however there are still some minors details that you should notice. Well done!
src/main/java/duke/Parser.java
Outdated
public void todo(String[] input, TaskList taskList) throws DukeException { | ||
if (input.length == 1) { | ||
throw new DukeException("todo needs a description"); | ||
} | ||
Task t = new Todo(input[1].strip()); | ||
taskList.addTask(t); | ||
System.out.println("Added new todo:\n " + t + "\nNumber of tasks: " + taskList.size()); | ||
} |
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 method name be "addTodo"?
src/main/java/duke/TaskList.java
Outdated
import duke.task.Task; | ||
|
||
public class TaskList { | ||
private ArrayList<Task> taskList; |
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 be "tasks"?
src/main/java/duke/Parser.java
Outdated
switch (input[0]) { | ||
case "list": | ||
taskList.list(); | ||
break; | ||
case "mark": | ||
mark(input, taskList); | ||
break; | ||
case "unmark": | ||
unmark(input, taskList); | ||
break; | ||
case "todo": | ||
todo(input, taskList); | ||
break; | ||
case "deadline": | ||
deadline(input, taskList); | ||
break; | ||
case "event": | ||
event(input, taskList); | ||
break; | ||
case "delete": | ||
delete(input, taskList); | ||
break; | ||
default: | ||
throw new DukeException("Sorry I do not understand the 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.
should the switch and case and default be on the same indentation?
src/main/java/duke/Parser.java
Outdated
} | ||
|
||
public void event(String[] input, TaskList taskList) throws DukeException { | ||
if (input.length == 1 || !input[1].contains("/from") || !input[1].contains("/to") ) { |
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 it be broken down to be three boolean variables?
public String getFileDesc() { | ||
return this.isDone | ||
? "D|1|" + this.name + "|" + convertFileDate(this.deadline) | ||
: "D|0|" + this.name + "|" + convertFileDate(this.deadline); | ||
} |
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 your break down line for easier reading
Assertions make it easier to ensure that certain variables/ methods are behaving as expected. By using assertions, we can make debugging easier as assumptions are ensured.
Add assertions
There are several methods that use complicated expressions. `mark`, `unmark`, `deadline` and `delete` methods in Parser class has 2 checks in an if condition. Abstracted the checks to 2 variables to enhance readability. `event` method in the Parser class has 3 checks in the if condition. Abstracted the checks to 3 variables to enhance readability. As a step forward, let's abstract complicated checks in the conditions.
Branch a code quality
Allow users to change the /by date of deadline tasks and /from, /to of event tasks.
DukeBot
DukePro frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useWhere do I begin?
All you need to do is,
And it is FREE!
Features