[Agamjyot] IP #508
[Agamjyot] IP #508
Conversation
There was a problem hiding this comment.
I've only commented out violations of code quality guidelines, and I believe there are some coding standards to correct as well.
Also, not very sure if all the extra lines after the method and class declarations are necessary. It might save a lot more LoC if the empty lines were omitted for shorter code that is easier to read. 😄
src/main/java/ChatBot.java
Outdated
|
|
||
| class ChatBot { | ||
|
|
||
| private String name; |
There was a problem hiding this comment.
Nothing major, but I feel like naming it botName would be clearer to understand, since later on there will be many possible instances of "names of tasks" and such.
src/main/java/Duke.java
Outdated
| chatBot.addTask(new Deadline(command[0], command[1])); | ||
| break; | ||
| case "event": | ||
| command = sc.nextLine().split(" /at "); | ||
| chatBot.addTask(new Events(command[0], command[1])); |
There was a problem hiding this comment.
Perhaps it would be better to abstract out command[0] and command[1] by doing:
keyword = command[0]orfirstWord = command[0]restOfCommand = command[1]
before you pass it into your Class Constructors for greater clarity. 😄
There was a problem hiding this comment.
yes, thank you for pointing that out, will make the changes in future commits.
src/main/java/ChatBot.java
Outdated
|
|
||
| public void markDone(int index) { | ||
|
|
||
| this.tasks.get(index).done(true); |
There was a problem hiding this comment.
Since you have a relatively long chain of code to run, it might be better to omit the this at the start of the code and just use tasks.get(index).done(true) instead. This also applies to all the instances of this.tasks you used in this ChatBot Class.
src/main/java/Task.java
Outdated
| return (isDone ? "X" : " "); // mark done task with X | ||
| } | ||
|
|
||
| public void done(boolean done) { |
There was a problem hiding this comment.
Isn't this method a little confusing, since it has the same name as the variable being passed inside? It might be better if it was called markTaskDone() or markDone() in short. The variable passed could be changed to boolean taskCompletionStatus for greater clarity as well.
There was a problem hiding this comment.
yes, naming the method as markTaskAsDone or markDone() will be better than just naming it done. I will make the necessary changes
src/main/java/Storage.java
Outdated
| while(sc.hasNextLine()) { | ||
|
|
||
| String task = sc.nextLine(); | ||
| char type = task.charAt(0); |
There was a problem hiding this comment.
type could be more clearly specified to be taskType.
src/main/java/Storage.java
Outdated
| taskList.add(new ToDo(description[0], isDone)); | ||
| break; | ||
| case 'D': | ||
| taskList.add(new Deadline(description[0], isDone, description[1])); | ||
| break; | ||
| case 'E': | ||
| taskList.add(new Events(description[0], isDone, description[1])); |
There was a problem hiding this comment.
Similarly, you could abstract out
description[0]to betaskName = description[0]anddescription[1]to berestOfDescription = description[1]
before passing them into the respective Constructors.
src/main/java/Task.java
Outdated
| public Task(String description, boolean isDone) { | ||
|
|
||
| this.description = description; | ||
| this.isDone = isDone; | ||
| } |
There was a problem hiding this comment.
Is this Constructor overload really necessary? From my understanding, all Tasks created should, by default, not be completed yet.
There was a problem hiding this comment.
Initially all tasks are created by default has not having been completed, but when you pass a task to mark() method then we also need to provide the boolean of the task has been done.
There was a problem hiding this comment.
Ah okay, I realised you are creating a new Task with this constructor in the Storage Class when you are reading in the duke.txt file to create the new Task instance based on whether the Task was marked done or not. I guess this could work then.
What I did was to create a default undone Task, then just subsequently use an if else statement to check for '1' or '0' and simply call the respective markDone() and markUndone() methods. This can remove the need for a Constructor overload 🙂
Add exception hanndling behaviour for deadline and event command
Add exception handling behaviour for todo and find command
Remove the extra new line at the end
Change return value when no task matches the keyword given
Was unable to create the data file when running the jar file
No description provided.