-
Notifications
You must be signed in to change notification settings - Fork 590
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
[chashaobao] iP #640
base: master
Are you sure you want to change the base?
[chashaobao] iP #640
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.
src/main/java/LittleMissHelpful.java
Outdated
loadTasks(list); | ||
|
||
int counter = list.size();; | ||
while (true) { // Continuous 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.
Would it be better to refactor this part into the the Parse class as suggested in the project brief week 3?
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.
agreed! Separating all the parsing commands into a separate parse class could make the code look neater
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.
Yes, thank you! I have abstracted out the classes now
src/main/java/LittleMissHelpful.java
Outdated
System.out.println("Ok, I see you laze. You have marked this task as not done yet: " + curTask.toString()); | ||
System.out.println(lineBreak); | ||
} catch (NumberFormatException e) { | ||
System.out.println(lineBreak + "\nHai-yo, task number must be an integer lah.\n" + lineBreak); |
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 check looks the same as the check for the "mark" command, perhaps can refactor to reduce repetition?
src/main/java/LittleMissHelpful.java
Outdated
Task newTask = null; | ||
|
||
try { | ||
if (command.equalsIgnoreCase("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.
The switch and case statement may come in handy here. (frankly I also never use)
src/main/java/LittleMissHelpful.java
Outdated
} else { | ||
// Invalid command --> throw exception | ||
System.out.println( | ||
lineBreak + "\nAlamak! Format salah already... Please provide a valid command and description.\n" + lineBreak); |
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 can provide some descriptions of what are valid commands
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,18 @@ | |||
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.
Would it make sense to save the string as Java Datetime instead?
src/main/java/LittleMissHelpful.java
Outdated
public class LittleMissHelpful { | ||
private static final String FILE_PATH = "./data/LittleMissHelpfullist.txt"; | ||
public static void main(String[] args) { | ||
/** |
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 can add this comment in the header instead? (i.e. before the public class in Line 10) It may help others to understand your code purpose better!
src/main/java/LittleMissHelpful.java
Outdated
loadTasks(list); | ||
|
||
int counter = list.size();; | ||
while (true) { // Continuous 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.
agreed! Separating all the parsing commands into a separate parse class could make the code look neater
src/main/java/LittleMissHelpful.java
Outdated
|
||
if (inputList.length < 2) { | ||
// Check if the user command is "bye" -> exit | ||
if (command.equalsIgnoreCase("bye")) { |
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 separate the UI related code to a UI class?
src/main/java/Task.java
Outdated
@@ -0,0 +1,112 @@ | |||
public class 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.
Could you consider adding header comments to the classes? Saw this problem in some other classes too.
src/main/java/Task.java
Outdated
protected String description; | ||
protected boolean isDone; | ||
|
||
public Task (String description) { |
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.
An idea is that you could make this into an abstract class and have the different methods for the different classes in their respective class files! It's just an alternative way to do it that you can consider.
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.
In general, I think your code is really well-organised and well-abstracted. This is the first time I've seen someone create a Java file for each command. I also like the vibes your bot's responses give. Good job! LGTM.
@@ -0,0 +1,59 @@ | |||
package main.LittleMissHelpful; | |||
import java.util.ArrayList; |
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.
Small suggestion involving code consistency but I noticed some files have a line of space between the package
statement and the import
statement while other files don't. The course's coding standard doesn't really prescribe a set way to go about this, but perhaps you could consider either using one or the other.
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.
Thank you for your feedback! I'll be sure to edit this
import main.LittleMissHelpful.Task.Task; | ||
|
||
public class Ui { | ||
private static final String LINE_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.
Good practice to make the divider a constant so that you don't have to repeat yourself whenever you use it.
if (parts.length < 3) { | ||
throw new InvalidTaskFormatException("Invalid task format: " + taskString); | ||
} | ||
|
||
String taskType = parts[0]; | ||
boolean isDone = parts[1].equals("1"); | ||
String description = parts[2]; | ||
|
||
// Check task type and create corresponding Task | ||
if (taskType.equals("T")) { | ||
return new Todo(description, isDone); | ||
|
||
} else if (taskType.equals("D")) { | ||
if (parts.length != 4) { | ||
throw new InvalidTaskFormatException("Invalid Deadline format: " + taskString); | ||
} | ||
LocalDateTime by = LocalDateTime.parse(parts[3]); | ||
return new Deadline(description, isDone, by); | ||
|
||
} else if (taskType.equals("E")) { | ||
if (parts.length != 5) { | ||
throw new InvalidTaskFormatException("Invalid Event format: " + taskString); | ||
} | ||
|
||
LocalDateTime from = LocalDateTime.parse(parts[3]); | ||
LocalDateTime to = LocalDateTime.parse(parts[4]); | ||
return new Event(description, isDone, from, 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.
Could possibly refactor the if-else statement into a switch-case statement (not 100% necessary but a suggestion).
if (this instanceof Todo) { | ||
taskType = "T"; | ||
description = this.description; | ||
return taskType + spacer + isDone + spacer + description; | ||
|
||
} else if (this instanceof Deadline d) { | ||
taskType = "D"; | ||
description = d.description; | ||
by = d.by; | ||
return taskType + spacer + isDone + spacer + description + spacer + by; | ||
|
||
} else if (this instanceof Event e) { | ||
taskType = "E"; | ||
description = e.description; | ||
from = e.from; | ||
to = e.to; | ||
return taskType + spacer + isDone + spacer + description + spacer + from + spacer + to; | ||
} else { | ||
throw new InvalidTaskFormatException("Cannot write task: " + this.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.
Was wondering if the switch-case syntax would be more idiomatic here too
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! The naming for class , method and variables is well done and convey their purposes clear!
Would be nice if header comments could be added to class and the public method as part of JavaDoc!
Well done!
import LittleMissHelpful.Ui; | ||
import LittleMissHelpful.Exception.InvalidCommandException; | ||
import LittleMissHelpful.Task.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.
The header comment for the class might be missed out, will be good if it could be added in
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.
The header comment for the class and the public methods is also missed out here, would be nice to add in!
if (dateTimeString.equalsIgnoreCase("today")) { | ||
// Use current date and set time to 00:00 (start of the day) | ||
return LocalDateTime.now().withHour(00).withMinute(00); | ||
} |
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 breaks could be added here to seperate the section and make the code more readabel : )
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.
missing header comment for the class too
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.
same as above
switch (command) { | ||
case "mark": | ||
return new MarkTaskCommand(Integer.parseInt(item)); | ||
|
||
case "unmark": | ||
return new UnmarkTaskCommand(Integer.parseInt(item)); | ||
|
||
case "delete": | ||
return new DeleteTaskCommand(Integer.parseInt(item)); | ||
|
||
case "find": | ||
return new FindCommand(item); | ||
|
||
case "todo": | ||
return new AddTodoCommand(item); | ||
|
||
case "deadline": | ||
String[] deadlineParts = item.split("/by", 2); | ||
if (deadlineParts.length < 2) { | ||
throw new InvalidCommandException("Invalid deadline format. Use 'deadline description /by date'."); | ||
} | ||
String deadlineDesc = deadlineParts[0].trim(); | ||
String deadlineBy = deadlineParts[1].trim(); | ||
return new AddDeadlineCommand(deadlineDesc, deadlineBy); | ||
|
||
case "event": | ||
String[] eventParts = item.split("/from | /to", 3); | ||
if (eventParts.length < 3) { | ||
throw new InvalidCommandException("Invalid event format. Use 'event description /from start /to end'."); | ||
} | ||
String eventDesc = eventParts[0].trim(); | ||
String eventFrom = eventParts[1].trim(); | ||
String eventTo = eventParts[2].trim(); | ||
return new AddEventCommand(eventDesc, eventFrom, eventTo); | ||
|
||
default: | ||
throw new InvalidCommandException("Invalid command. Please provide a valid 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.
the case
should not be indented, would be nice if it is fixed!
import LittleMissHelpful.Ui; | ||
import LittleMissHelpful.Exception.InvalidCommandException; | ||
import LittleMissHelpful.Task.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.
The commands naming is named well! Easy to understand what the class is handling!
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.
The exceptions class name is named well to let developer understand what exception it is handling too! Well done!
} | ||
} | ||
|
||
String item = inputList[1]; |
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 item could be named to taskIndex
for better understanding?
// public Event(String description, LocalDate from, LocalDate to) { | ||
// /** | ||
// * Instatiate event without needing to input time | ||
// * Defaults from time to start of the day and end time to end of the day | ||
// */ | ||
// super(description); | ||
// this.from = from.atStartOfDay(); // Set default time to start of day | ||
// this.to = to.atStartOfDay().plusDays(1).minusSeconds(1); // Set end of day time | ||
// } | ||
|
||
// public Event(String description, boolean isDone, LocalDate from, LocalDate to) { | ||
// /** | ||
// * Instatiate event without needing to input time && isDone | ||
// * Defaults from time to start of the day and end time to end of the day | ||
// */ | ||
// super(description, isDone); | ||
// this.from = from.atStartOfDay(); // Set default time to start of day | ||
// this.to = to.atStartOfDay().plusDays(1).minusSeconds(1); // Set end of day time | ||
// } |
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 consider removing unused code to improve code quality
Assertions were added to LittleMissHelpful file to enhance clarity and debugging Let's, * add assertions to LittleMissHelpful * enable assertions in the configurations
Packages were not in full lowercase Let's * rename all packages to follow convention.
Add assertions
Branch a code quality
add date and time for deadline and event
LittleMissHelpful
Ah Bang Mang frees your mind from remembering the things you need to do. It's:
All you need to do is:
And the best part? It’s FREE!
Features:
If you are a Java programmer, you can use LittleMissHelpful to practice Java too! Here's the main method: