-
Notifications
You must be signed in to change notification settings - Fork 429
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
[geeliette] iP #455
base: master
Are you sure you want to change the base?
[geeliette] iP #455
Conversation
In build.gradle, the dependencies on distZip and/or distTar causes the shadowJar task to generate a second JAR file for which the mainClass.set("seedu.duke.Duke") does not take effect. Hence, this additional JAR file cannot be run. For this product, there is no need to generate a second JAR file to begin with. Let's remove this dependency from the build.gradle to prevent the shadowJar task from generating the extra JAR file.
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.
Looks good overall! Also liked how unique the responses are!
src/main/java/fleur/Parser.java
Outdated
if (str.equalsIgnoreCase("bye")) { | ||
exit(); | ||
} else if (str.equalsIgnoreCase("list")) { | ||
this.taskList.listTasks(); | ||
} else if (str.startsWith("mark")) { | ||
int index = Integer.parseInt(str.substring(5)) - 1; | ||
this.taskList.markDone(index); | ||
System.out.println("Enchanté! I 'ave marked zis task as done:"); | ||
System.out.println(this.taskList.getTask(index).toString()); | ||
} else if (str.startsWith("unmark")) { | ||
int index = Integer.parseInt(str.substring(7)) - 1; | ||
this.taskList.markUndone(index); | ||
System.out.println("Zut! I 'ave marked zis task as not done:"); | ||
System.out.println(this.taskList.getTask(index).toString()); | ||
} else if (str.startsWith("todo")) { | ||
addToDo(str); | ||
} else if (str.startsWith("deadline")) { | ||
addDeadline(str); | ||
} else if (str.startsWith("event")) { | ||
addEvent(str); | ||
} else if (str.startsWith("delete")) { | ||
int index = Integer.parseInt(str.substring(7)) - 1; | ||
Task deletedTask = this.taskList.getTask(index); | ||
System.out.println("D'accord, I 'ave removed zis task from your list:"); | ||
System.out.println(deletedTask.toString()); | ||
this.taskList.deleteTask(index); | ||
System.out.println("Now you 'ave " + this.taskList.size() + " task(s) in your list."); | ||
} else { | ||
throw new FleurInvalidCommandException(); | ||
} |
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 using switch case statements could make the code easier to read?
src/main/java/fleur/Storage.java
Outdated
switch (str.charAt(1)) { | ||
case 'T': | ||
t = new ToDo(str.substring(7)); | ||
break; | ||
case 'D': | ||
String desc = str.substring(7).split("\\(by: ")[0]; | ||
String date = str.substring(7).split("\\(by: ")[1].replace(")", ""); | ||
LocalDate by = LocalDate.parse(date, INPUT); | ||
t = new Deadline(desc, by); | ||
break; | ||
case 'E': | ||
String[] arr = str.substring(7).split("\\(from: "); | ||
String description = arr[0]; | ||
String from = arr[1].split("to: ")[0].trim(); | ||
String to = arr[1].split("to: ")[1].replace(")", ""); | ||
LocalDate dateFrom = LocalDate.parse(from, INPUT); | ||
LocalDate dateTo = LocalDate.parse(to, INPUT); | ||
t = new Event(description, dateFrom, dateTo); | ||
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.
Just a minor layout fix; remember not to have indentations for case clauses in this course!
src/main/java/fleur/Ui.java
Outdated
this.scanner = new Scanner(System.in); | ||
} | ||
|
||
public void welcomeMessage() { |
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 using a verb for the method name, e.g. showWelcomeMessage(), could make it more intuitive.
src/main/java/fleur/Storage.java
Outdated
String desc = str.substring(7).split("\\(by: ")[0]; | ||
String date = str.substring(7).split("\\(by: ")[1].replace(")", ""); | ||
LocalDate by = LocalDate.parse(date, INPUT); | ||
t = new Deadline(desc, by); | ||
break; | ||
case 'E': | ||
String[] arr = str.substring(7).split("\\(from: "); |
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 using full/more descriptive words for the variable names 'desc' and 'arr' would make it easier for readers to understand what they are used for.
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.
Looks good overall! Love the creativity in the use of French in your responses.
Do remember to add javadoc comments.
src/main/java/fleur/Storage.java
Outdated
this.dataFile = dataFile; | ||
} | ||
|
||
protected ArrayList<Task> loadData() throws IOException, FleurCorruptFileException { |
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.
protected
Why do you choose to use protected here? Will there be any class that will inherit from Storage and need access to this method?
src/main/java/fleur/Fleur.java
Outdated
// System.out.println(e.getMessage()); | ||
// } | ||
// } | ||
// } | ||
// | ||
// private void listTasks() { | ||
// if (tasks.isEmpty()) { | ||
// System.out.println("You 'ave non tasks in your list."); | ||
// } else { | ||
// System.out.println("'Ere are all ze tasks in your list:"); | ||
// for (int i = 0; i < count; i++) { | ||
// System.out.println((i + 1) + "." + tasks.get(i).toString()); | ||
// } | ||
// } | ||
// } | ||
// | ||
// private void markDone(int index) { | ||
// tasks.get(index).markAsDone(); | ||
// System.out.println("Enchanté! I 'ave marked zis task as done:"); | ||
// System.out.println(tasks.get(index).toString()); | ||
// } | ||
// | ||
// private void markUndone(int index) { | ||
// tasks.get(index).markAsUndone(); | ||
// System.out.println("Zut! I 'ave marked zis task as not done:"); | ||
// System.out.println(tasks.get(index).toString()); | ||
// } | ||
// | ||
// private void addToDo(String str) throws FleurMissingDetailsException { | ||
// try { | ||
// String desc = str.substring(5); | ||
// Task t = new ToDo(desc); | ||
// tasks.add(t); | ||
// count++; | ||
// System.out.println("Bah, oui! I 'ave added zis todo task to your list:"); | ||
// System.out.println(t.toString()); | ||
// System.out.println("Now you 'ave " + count + " task(s) in your list."); | ||
// } catch (StringIndexOutOfBoundsException e) { | ||
// throw new FleurMissingDetailsException(); | ||
// } | ||
// } | ||
// | ||
// private void addDeadline(String str) throws FleurMissingDetailsException, FleurInvalidDateException { | ||
// try { | ||
// String desc = str.substring(9).split("/by")[0]; | ||
// String date = str.substring(9).split("/by")[1].trim(); | ||
// LocalDate by = LocalDate.parse(date, INPUT); | ||
// Task t = new Deadline(desc, by); | ||
// tasks.add(t); | ||
// count++; | ||
// System.out.println("Bah, oui! I 'ave added zis deadline task to your list:"); | ||
// System.out.println(t.toString()); | ||
// System.out.println("Now you 'ave " + count + " task(s) in your list."); | ||
// } catch (IndexOutOfBoundsException e) { | ||
// throw new FleurMissingDetailsException(); | ||
// } catch (DateTimeParseException e) { | ||
// throw new FleurInvalidDateException(); | ||
// } | ||
// } | ||
// | ||
// private void addEvent(String str) throws FleurMissingDetailsException, FleurInvalidDateException { | ||
// try { | ||
// String[] arr = str.substring(6).split("/from"); | ||
// String desc = arr[0]; | ||
// String from = arr[1].split("/to")[0].trim(); | ||
// String to = arr[1].split("/to")[1].trim(); | ||
// LocalDate dateFrom = LocalDate.parse(from, INPUT); | ||
// LocalDate dateTo = LocalDate.parse(to, INPUT); | ||
// Task t = new Event(desc, dateFrom, dateTo); | ||
// tasks.add(t); | ||
// count++; | ||
// System.out.println("Bah, oui! I 'ave added zis event task to your list:"); | ||
// System.out.println(t.toString()); | ||
// System.out.println("Now you 'ave " + count + " task(s) in your list."); | ||
// } catch (ArrayIndexOutOfBoundsException | StringIndexOutOfBoundsException e) { | ||
// throw new FleurMissingDetailsException(); | ||
// } catch (DateTimeParseException e) { | ||
// throw new FleurInvalidDateException(); | ||
// } | ||
// } | ||
// | ||
// private void deleteTask(int index) { | ||
// Task deletedTask = tasks.get(index); | ||
// count--; | ||
// System.out.println("D'accord, I 'ave removed zis task from your list:"); | ||
// System.out.println(deletedTask.toString()); | ||
// System.out.println("Now you 'ave " + count + " task(s) in your list."); | ||
// tasks.remove(index); | ||
// } | ||
// | ||
// private void exit() { | ||
// try { | ||
// saveData(); | ||
// System.out.println("Au revoir, 'ope to see you again soon!"); | ||
// } catch (IOException e) { | ||
// System.out.println("Mince! Zere was an error when saving your tasks."); | ||
// } | ||
// } | ||
// | ||
//} |
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.
Having such a block of commented code may not be the best practice. Consider deleting it
src/main/java/fleur/Parser.java
Outdated
this.taskList.addTask(t); | ||
System.out.println("Bah, oui! I 'ave added zis event task to your list:"); | ||
System.out.println(t.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.
While this Task object is temporary, consider using a more descriptive name (e.g. taskToAdd
)
src/main/java/fleur/Parser.java
Outdated
} | ||
} | ||
|
||
private void addEvent(String str) throws FleurMissingDetailsException, FleurInvalidDateException { |
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 specifying the two checked Exceptions separately instead of just specifying throws FleurException
, so that the callers can choose to handle them separately.
src/main/java/fleur/Storage.java
Outdated
switch (str.charAt(1)) { | ||
case 'T': | ||
t = new ToDo(str.substring(7)); | ||
break; | ||
case 'D': | ||
String desc = str.substring(7).split("\\(by: ")[0]; | ||
String date = str.substring(7).split("\\(by: ")[1].replace(")", ""); | ||
LocalDate by = LocalDate.parse(date, INPUT); | ||
t = new Deadline(desc, by); | ||
break; | ||
case 'E': | ||
String[] arr = str.substring(7).split("\\(from: "); | ||
String description = arr[0]; | ||
String from = arr[1].split("to: ")[0].trim(); | ||
String to = arr[1].split("to: ")[1].replace(")", ""); | ||
LocalDate dateFrom = LocalDate.parse(from, INPUT); | ||
LocalDate dateTo = LocalDate.parse(to, INPUT); | ||
t = new Event(description, dateFrom, dateTo); | ||
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.
Remember to configure your IDE not to have indentations for case clauses for this course
delete 3 | ||
hi | ||
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.
Good job testing the edge cases and error messages! Consider testing the case of index out of bounds as well
package fleur; | ||
|
||
public class FleurException extends 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.
Perhaps you could group the Exception classes together into 1 package to make the file organisation even tidier
The current implementation does not enforce certain key assumptions about user input and task modifications. Task indices in commands like delete, mark, and unmark should be more than 0 and less than the total number of tasks. Additionally, parse() does not explicitly check for empty input strings before processing them. These missing validations can lead to unexpected runtime errors, causing the program to crash instead of failing early with a clear message. Let's add assert statements in critical parts of the code to ensure valid preconditions hold before execution. Specifically: * Add assertions in parse() to validate that input is non-null and non-empty * Introduce assertions in execute() method in DeleteCommand, MarkCommand, and UnmarkCommand to check that task indices are within a valid range
The loadData() method in Storage is too long and tightly coupled with the task creation logic. It attempts to parse raw text data, determine task types, and instantiate the correct task objects within a single method. The method contains more details than what the reader can process at a time, making it difficult to maintain and extend in the future. Refactoring loadData() by adding new methods in the Command classes allows us to abstract and move task creation logic to a more appropriate place, improving code quality and readability. Let's, * introduce createToDo(), createDeadline(), and createEvent() in AddToDoCommand, AddDeadlineCommand, and AddEventCommand * modify loadData() to delegate task creation to these methods instead of handling it directly
Add assert statements to enforce key assumptions in input validation
Refactor Storage and Command classes to improve task loading and saving
Fleur: Your Key To Blooming 🌸
Fleur is the system for you to keep track of all your daily tasks, so that you can achieve your goals more efficiently.
Why You'll Love Fleur
How To Use It
java -jar "fleur.jar"
And it's FREE!
Features
Main Method
If you are a Java programmer, you can use it to practice Java too. Here's the
main
method: