-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Shawn Tan Jing Kai] iP #486
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.
LGTM, just some small nits I pointed out in the comments. Perhaps u can consider having a task
package and putting all the files that extends Task
inside. Keep up the good work!
this.by = LocalDate.parse(by); | ||
} | ||
|
||
@Override |
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 remember to add Override annotations!
src/main/java/duke/Parser.java
Outdated
String dummyString; | ||
Task dummyTask; | ||
int counter; | ||
int start; | ||
int end; |
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 there be access modifiers for these fields? 🤔 Same with codes in other files.
System.out.println("Nice! I've marked this task as done:\n" + | ||
"[" + tasks.get(counter).getStatusIcon() + "] " + tasks.get(counter).getDescription()); | ||
storage.saveFile(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.
Perhaps u can remove the newline between if
and else if
statements etc? Same go with other lines of code.
Files.createDirectories(Paths.get("data")); | ||
this.file = new File("data/tasks.txt"); | ||
file.createNewFile(); | ||
} catch (IOException e) { |
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 this catchblock be empty? 💭
return "[" + this.getStatusIcon() + "] " + this.getDescription(); | ||
} | ||
|
||
String saveStringToFile() { |
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 access modifier!
|
||
public class EventTest { | ||
@Test | ||
public void getTaskTypeTest(){ |
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.
public void getTaskTypeTest(){ | |
public void getTaskTypeTest() { |
} | ||
|
||
@Test | ||
public void toStringTest(){ |
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.
public void toStringTest(){ | |
public void toStringTest() { |
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.
Your code generally looks good :D
There are some minute minor details that you might want to take note of
|
||
public class Deadline extends Task { | ||
|
||
protected LocalDate 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.
Perhaps you might want to consider a more descriptive variable name like byDate
, so that readers can easily differentiate between this variable and the other by
parameter in your constructor
|
||
public class Event extends Task { | ||
|
||
protected LocalDate at; |
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.
Similar to Deadline.java
, you could consider a more descriptive variable name like atDate
src/main/java/duke/Todo.java
Outdated
package duke; | ||
public class Todo 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.
You can consider removing this variable declaration since it's not being used
…java and parser.java
Assertions are not existent in the code to verify bugs. As such, certain bugs might not be detected in code. Use assert feature to document important assumptions that should hold at various points in the code. This is to prevent major bugs in the code such as description of Tasks being null.
Add assertions to code
# Conflicts: # src/main/java/duke/Parser.java
Duke is unable to provide a reminder function that can remind the user of upcoming events or deadlines in the coming week. Reminder function should be added so that user can easily know what are the upcoming events and deadlines. Let's have a reminder function that can allow the user to know about the upcoming deadlines and events within the next week. Typing in "Reminder" returns the deadlines and events for the next 7 days. E.g. Typing "Reminder" on 12 September 2022 will return the deadlines and events from 12 September 2022 (Exclusive) to 19 September 2022 (Exclusive).
Duke
Duke frees your mind of having to remember things you need to do. It's,
FASTSUPER FAST to useAll you need to do is,
And it is FREE!
Features:
If you are a Java programmer, you can use it to practice Java too. Here's the
main
method: