-
Notifications
You must be signed in to change notification settings - Fork 454
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
[Cheah Yan (Xie Yan)] iP #486
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 881bfc1.
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, the use of good naming conventions and meaningful variable names shows that you placed a lot of thought into how your files can be easily understood by others. Having comment headers would greatly enhance your work.
Overall, LGTM!
import java.time.LocalDateTime; | ||
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadline extends 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.
Perhaps a class should have a header comment to inform users about what the class does. Same suggestion for the other .java files
src/main/java/duke/Deadline.java
Outdated
|
||
protected LocalDateTime date; | ||
|
||
public Deadline(String description, String date) { |
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 a method could have a header comment to inform users about what the method does.
Same suggestion for the other methods both in this file and other .java files
@@ -0,0 +1,15 @@ | |||
package duke; | |||
|
|||
public class DukeException 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.
Great work using inheritance! LGTM!
src/main/java/duke/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
public String convertToFile() { |
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.
Great use of naming conventions!
src/main/java/duke/Duke.java
Outdated
protected static final String LOCAL_FILE = "data/duke.txt"; | ||
|
||
public static void appendToFile(String filePath, String textToAppend) throws IOException { | ||
FileWriter fw = new FileWriter(filePath, true); // create a FileWriter in append mode |
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.
leaving comments to help readers enables a better understanding of your code and might even help to hasten the onboarding process. Good job!
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, your code is structured logically which makes it easy to follow. I think the main issue to fix would the naming of a few variables and methods where it can be misleading or difficult to understand. Also, it would be great to include Javadoc comments which I believe would improve your code's readability. I hope my reviews will be useful to you! 🙂
src/main/java/duke/Deadline.java
Outdated
public Deadline(String description, String date) { | ||
super(description); | ||
|
||
DateTimeFormatter scanned = DateTimeFormatter.ofPattern("dd/MM/yyyy HHmm"); |
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 a more meaningful name for this variable would be datePattern or dateFormat.
src/main/java/duke/Duke.java
Outdated
String a = sc.nextLine(); | ||
String[] b = a.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.
Perhaps it would be good if you could avoid using single letters to name variables unless they are iterator variables. I noticed this issue in other parts too.
src/main/java/duke/Duke.java
Outdated
|
||
} else if (details[0].equals("delete")) { | ||
int taskIndex = Integer.valueOf(details[1]); | ||
Task removed = history.get(taskIndex - 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.
Perhaps a more meaningful name would be removedTask to indicate that this is the task that had been removed.
src/main/java/duke/Task.java
Outdated
this.isDone = false; | ||
} | ||
|
||
public String convertToFile() { |
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 a more suitable name is getDescription. I feel that naming it convertToFile can be misleading since this method does not handle any conversion.
sc.close(); | ||
|
||
} | ||
} |
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 apply more OOP by extracting out closely related code as classes, rather than letting the Main method handle everything.
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.
Great work overall :)
src/main/java/duke/Ui.java
Outdated
System.out.println("Bye! Hope to see you again soon!"); | ||
} | ||
|
||
public void list(TaskList 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.
I believe the coding standards suggest that method names should be verbs. In this case, 'list' could be a noun. Maybe a better option would be to name the method 'listAll'.
src/main/java/duke/Task.java
Outdated
return (isDone ? "X" : " "); | ||
} | ||
|
||
public void Done() { |
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.
Method names should be in camel case.
src/main/java/duke/Parser.java
Outdated
return this.twoPart; | ||
} | ||
|
||
public boolean isBye() { |
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 guess u can shorten this into one-liner by just returning 'this.command.equals("bye"')'. Helps improve readability somewhat but I guess I'm just nitpicking here.
src/main/java/duke/Parser.java
Outdated
return this.twoPart[0]; | ||
} | ||
|
||
public int secondPartInInt() throws DukeException { |
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.
Method names should be verbs. So this would be better named as 'getSecondPartInInt'.
Program does not have Assertions Assertions help to indicate a possible bug in the code at runtime. Adding assertions will allow us to more easily identify mistakes that we programmers make in the code.
GUI does not display the full TaskList if there is a large number of tasks. Users will not be able to properly view the entire list of tasks easily DialogBox.fxml file changed allows us to easily view all the tasks in the list when "list" command is called.
Current code is not very readable for other programmers. This can be very hard for programmers to improve upon the code in the future. Readability of code is improved with current commit
Add assertions
Improve Code Quality to all files
Added Reminder feature to Duke.
DukeLite
DukeLite 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 Java programmer, you can use it to practice Java too. Here's the
main
method: