-
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
[Rehad A] iP #474
base: master
Are you sure you want to change the base?
[Rehad A] iP #474
Conversation
src/main/java/DeadlineTask.class | ||
src/main/java/Duke.class | ||
src/main/java/EventTask.class | ||
src/main/java/Task.class |
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 be simplified as src/main/java/*.class
to make the file less verbose ?
src/main/java/DeadlineTask.java
Outdated
|
||
public DeadlineTask(String description, String time) { | ||
super(description); | ||
this.type = "D"; |
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 we remove this
as these variables are not shadowed
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
public static void loadTasksFromFile(ArrayList<Task> tasks) throws IOException, DukeException { | ||
String directoryPath = "./data"; |
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 we add visibility modifier such as private
for these variables
src/main/java/Duke.java
Outdated
@@ -1,10 +1,166 @@ | |||
import jdk.jfr.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.
Is this unused imports ?
src/main/java/Duke.java
Outdated
Scanner f = new Scanner(dataFile); | ||
|
||
while (f.hasNext()) { | ||
String[] task = f.nextLine().split("|"); |
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 we use plural form for task
src/main/java/Duke.java
Outdated
System.out.printf("%d.%s\n", i + 1, tasks.get(i)); | ||
} | ||
System.out.println(line); | ||
}else if (command.contains("delete")) { |
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 we fix the spacing here ?
src/main/java/Duke.java
Outdated
int taskIdxStart = command.indexOf(" ") + 1; | ||
String task = command.substring(taskIdxStart); | ||
tasks.add(new TodoTask(task)); | ||
System.out.printf("%sGot it. I've added this task:\n%s\nNow you have %d tasks in the list\n%s\n", line, tasks.get(ctr), ctr + 1, 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.
Should we separate this into 2 lines, as 1 line has max of 120 chars
src/main/java/Duke.java
Outdated
} | ||
|
||
public static void saveTasksToFile(ArrayList<Task> tasks) throws IOException { | ||
FileWriter fw = new FileWriter("./data/duke.txt"); |
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.
Really nice use of file writer btw! just one suggestion you can check try-with-resources
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 pretty clean code and functional! Just maybe more spacing and newlines so that people can read it easier
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 implementation overall!
src/main/java/Duke.java
Outdated
try { | ||
if (command.contains("done")) { | ||
if (!command.contains(" ")) { | ||
throw new DukeException("OOPS!! done needs the index of the 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.
Should we use an alternative implementation that reduces nesting?
src/main/java/Duke.java
Outdated
System.out.println(line + "Hello I'm Duke\nWhat can I do for you?\n" + line); | ||
|
||
ArrayList<Task> tasks = new ArrayList<>(100); | ||
int ctr = 0; |
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 we use a name that is more semantically appropriate for the variable?
src/main/java/EventTask.java
Outdated
this.type = "E"; | ||
this.time = time; | ||
} | ||
|
||
public EventTask(String description, boolean isDone, String time) { | ||
super(description, isDone); | ||
this.type = "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.
Seems like there is repetition in the assignment of this.type = "E"
. Perhaps this could be abstracted for ease of refactoring in the future?
src/main/java/Duke.java
Outdated
String taskType = task[0]; | ||
boolean isDone = (Integer.parseInt(task[1]) == 1); | ||
String description = task[2]; | ||
String time = task.length == 4 ? task[3] : 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.
Perhaps 4
could be defined as a class level constant to avoid the use of magic literals?
Duke only returns a bye message to the user without exiting the application. Let's update Duke to exit the application after sending the bye message
Add assertions
# Conflicts: # src/main/java/duke/Storage.java
# Conflicts: # src/main/java/duke/Storage.java
Duke
Duke helps you in managing and organizing your tasks. It's,
FASTSUPER FAST to useAll you need to do is,
And it's FREE! 🥳 🎉
Features:
If you're a Java programmer, you can use it to practice Java too. Here's
main
method: