-
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
Mustafa Abbas iP #467
base: master
Are you sure you want to change the base?
Mustafa Abbas iP #467
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.
Overall, i think you have several coding standard violations that you may want to address, and you may consider breaking your Main class up into different classes to handle specific logic like a Parser class to handle making sense of user input and a Response/Ui class to handle printing out messages
src/main/java/Carol.java
Outdated
out.println(INTRO_MSG); | ||
list = loadTasks(); | ||
BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); | ||
boolean running = true; |
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 its might be better to use "isRunning" instead
src/main/java/Carol.java
Outdated
@@ -0,0 +1,259 @@ | |||
import java.io.*; |
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.
Imported classes should be listed explicity
src/main/java/Carol.java
Outdated
String eventTimeStart = eventStartString.split(" ")[1]; | ||
String eventDateEnd = eventEndString.split(" ")[0]; | ||
String eventTimeEnd = eventEndString.split(" ")[1]; | ||
Event ev = new Event(eventMessage, LocalDate.parse(eventDateStart), LocalDate.parse(eventDateEnd), LocalTime.parse(eventTimeStart), LocalTime.parse(eventTimeEnd)); |
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 length should be no longer than 120 chars, you could try to bring some arguments into the next line
src/main/java/Task.java
Outdated
return null; | ||
} | ||
|
||
} |
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.
A suggestion i have is to allow each Task class (Todo, Deadline, Event) to handle the logic of parsing for their specific task type.
DateTimeFormatter.ofPattern("hh:mm a") // 12-hour format with AM/PM (05:30 PM) | ||
}; | ||
|
||
public static Task parseTaskFromFile(String line) throws CarolException { |
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.
Though the purpose of the function is clear, DocStrings are still required for every function
src/main/java/carol/io/Ui.java
Outdated
Hello! I'm Carol, your personal assistant. | ||
What can I do for you? | ||
"""; | ||
protected final String END_MSG = " Bye! Hope to hear from you soon!\n"; |
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 used SCREAMING_SNAKE_CASE to define your constants
src/main/java/carol/io/Ui.java
Outdated
System.out.println(message); | ||
} | ||
|
||
public static void line() { |
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 function needs a better name. Consider using printLine instead
import java.time.format.DateTimeParseException; | ||
|
||
public class EventParser { | ||
private static final DateTimeFormatter[] DATE_FORMATS = { |
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 not using an array for this. Makes code extremely hard to read, as you have to reference what DATE_FORMATS[0] is every time you see it. Maybe do a yyyyMMddFormatter as a name instead.
src/main/java/tasks/Task.java
Outdated
} | ||
|
||
public void markAsDone() { | ||
if (!isDone) { |
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 just isDone = true; is enough
src/main/java/tasks/ToDo.java
Outdated
package tasks; | ||
|
||
public class ToDo extends Task{ | ||
private String type; |
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.
String type is not so necessary, or you may change it to final field for safe.
src/main/java/tasks/Event.java
Outdated
import java.time.LocalTime; | ||
|
||
public class Event extends Task { | ||
private String type; |
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.
make String type to be final could be better
switch (taskType) { | ||
case "T": | ||
ToDo todo = new ToDo(description); | ||
if (isDone) { |
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 provide another constructor Todo(String description, Boolean isDone)
LocalTime time = LocalTime.parse(deadlineString, TIME_FORMATS[0]); | ||
Deadline deadline = new Deadline(description, date, time); | ||
if (isDone) { | ||
deadline.markAsDone(); |
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 provide another constructor Deadline(String description, LocalDate date, LocalTime time, Boolean isDone)
LocalTime eventTimeEnd = LocalTime.parse(eventEndParts[1]); | ||
|
||
Event event = new Event(description, eventDateStart, eventDateEnd, eventTimeStart, eventTimeEnd); | ||
if (isDone) { |
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 with deadline and event.
Loading tasks from storage leads to incorrect isDone value being returned Format of LocalDate and LocalTime Strings in Deadline and Event instances is not standardised Standardised DateTime values allows for users to read values easier Correct isDone value ensure user knows when they have marked the task as done Let's ensure that tasks are displayed and stored correctly
Increase code readability
Add HelpCommand feature
Fix DateTime parsing to provide correct time
π Cortana β Your AI Task Manager
Cortana is your AI-powered personal assistant that helps you manage tasks seamlessly. It's:
β¨ Getting Started
And best of allβ¦ it's FREE! π
π Features
Task Capabilities
Todo
Deadline
Event
π₯οΈ Java Programmers, Here's the Entry Point