-
Notifications
You must be signed in to change notification settings - Fork 484
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
[Chen-Kuei] iP #577
base: master
Are you sure you want to change the base?
[Chen-Kuei] iP #577
Conversation
text-ui-test/runtest.bat|sh scripts generate a file ACTUAL.TXT. However, .gitignore uses ACTUAL.txt, which means the generated file will not be ignored by Git on non-Windows OS. Let's update .gitignore as ACTUAL.txt -> ACTUAL.TXT
This reverts commit d8a0c18.
This reverts commit 59487b8.
This reverts commit 965afec.
This reverts commit 83e9a44.
# Conflicts: # src/main/java/duke/Deadline.java # src/main/java/duke/Duke.java # src/main/java/duke/Event.java # src/main/java/duke/Parser.java # src/main/java/duke/Storage.java # src/main/java/duke/Task.java # src/main/java/duke/TaskList.java # src/main/java/duke/Todo.java # src/main/java/duke/Ui.java
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! Here are a few recommendations
- add javadocs!
- idk, great job on this!
docs/duke.txt
Outdated
TODO | read book | ||
DEADLINE | project | 2022-10-10 | ||
EVENT | meet | 2023-06-12 1400 | 2023-06-12 1600 |
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.
correct me if I'm wrong, but I believe that this file should be added to your .gitignore file
src/main/java/duke/Ui.java
Outdated
String taskTime1 = (taskData.length > 2) ? taskData[2] : ""; | ||
String taskTime2 = (taskData.length > 3) ? taskData[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.
not very sure what these two task times represent? some commented code would be appreciated
src/main/java/duke/Ui.java
Outdated
storage.saveTasks(tasks); | ||
break; | ||
case "unmark": | ||
int notDoneTaskIndex = Integer.parseInt(parts[1]) - 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.
great job on writing a clear variable name!
private LocalDateTime startTime; | ||
private LocalDateTime endTime; |
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.
nice to see that you went beyond what was asked in the iP and made Event have LocalDateTime fields as well. However, this causes some user experience problems, because now in order to enter a Event task you have to write a whole big chunk of text 😬
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.
Well done! Good and clear code with accurate descriptions and good naming mechanisms. Can remove parts of code that make it look like a draft
src/main/java/duke/Duke.java
Outdated
// saveTask(); | ||
// } | ||
// } | ||
//} |
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.
Remove commented code. As long as this is a previous version, it will be saved in version control, or store in a separate txt file.
src/main/java/duke/Duke.java
Outdated
TODO | read book | ||
DEADLINE | project | 2022-10-10 | ||
EVENT | meeting | 2023-06-12 1400 | 2023-06-12 1800 | ||
*/ |
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.
State todo on top, delete when 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.
Overall a well done and concise piece of code. Good job! 😄
String[] commandParts = userCommand.split(" ", 2); | ||
String taskType = commandParts[0].toLowerCase(); | ||
|
||
switch (taskType) { |
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.
Cases should be in the same indentation as the switch keyword.
src/main/java/duke/Deadline.java
Outdated
* @return The parsed deadline date as a LocalDate object, or null if parsing fails. | ||
*/ | ||
private LocalDate parseDeadline(String deadlineString) { | ||
|
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'm not sure if this is an accident but there should be consistency in whether you separate your block statements with 1 line spacing or without.
Removing redundant code in each java class and correcting the indentation for 'case' clauses improve the code quality. Let's remove the indentation of 'case' clauses.
use assert feature
Improve code quality
# Conflicts: # src/main/java/duke/Duke.java
Hi! I'm BiuBiu :D
BiuBiu frees your mind of having to remember things you need to do. It's,
FAST SUPERFAST 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: