-
Notifications
You must be signed in to change notification settings - Fork 437
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
[Vaishak Anand] iP #248
base: master
Are you sure you want to change the base?
[Vaishak Anand] iP #248
Conversation
src/main/java/Command.java
Outdated
protected Task task; | ||
protected String message; | ||
|
||
public Command(String name) { |
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.
Think you can create subclasses for Command such as ListCommand / DeleteCommand to handle the various inputs. Should help to make the code more readable and better abstracted. Think that will help to optimize and shorten your code, and prevent having too many constructors for similar purposes
src/main/java/Deadline.java
Outdated
@Override | ||
public String taskSaver() { | ||
String type = "D"; | ||
return type + "/" + super.taskSaver() + "/" + by.format(DateTimeFormatter.ISO_LOCAL_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.
No issues, but just a suggestion to use String.format
src/main/java/Duke.java
Outdated
Scanner input = new Scanner(System.in); | ||
|
||
// Uses user input text and acts accordingly. | ||
loop: |
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 have never seen this syntax before? Is it an meant to be commented out?
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.
No it is not! It was meant to break out of the outer while loop from within the switch case, as a normal break
would only break out of the switch case.
Read this for more info: https://stackoverflow.com/questions/22823395/java-how-can-i-break-a-while-loop-under-a-switch-statement
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.
Wow, thanks for sharing man
src/main/java/Duke.java
Outdated
Command command = ui.parseCommand(line); | ||
ui.lineBreak(); | ||
|
||
switch (command.name) { |
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 switch case, very readable!
src/main/java/Event.java
Outdated
*/ | ||
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.
I think you can afford to use "private" instead of "protected" since there is not subclass for Event.
src/main/java/Task.java
Outdated
public class Task { | ||
protected String description; | ||
protected boolean isDone; | ||
|
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.
Just a suggestion: I used enum to create TaskType and use Tasktype as part of the printing (further leverage on Liskov Sub Principle)
src/main/java/TaskList.java
Outdated
System.out.println("Good Job, this task is now done:"); | ||
System.out.println(toBeDone); | ||
} | ||
|
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.
Javadoc can be added here too
src/main/java/TaskList.java
Outdated
* Removes a task from the list of current tasks. | ||
* @param taskNo Index of task to remove. | ||
*/ | ||
void removeFromList(int taskNo) { |
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.
access modifier for your methods can be included
src/main/java/Ui.java
Outdated
*/ | ||
public class Ui { | ||
|
||
protected Parser parser; |
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 think can be private instead of protected as your Ui class has no subclass
src/main/java/Ui.java
Outdated
/** | ||
* Returns a line to the user. | ||
*/ | ||
public void lineBreak() { |
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.
Like the abstraction for readability!
Think overall your code has great names, but the access modifiers as well other minor errors can be improved upon! Great effort nonetheless :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.
Overall, I found your code quite structured and easy to read. I noted a few minor coding standard violations
too, usually is the naming of the class/variable.
src/main/java/Ui.java
Outdated
@@ -10,7 +10,7 @@ public Ui() { | |||
this.parser = new Parser(); | |||
} | |||
|
|||
public static String 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.
Since the line is not to share with other class, maybe change it to private instead?
src/main/java/Duke.java
Outdated
String line = input.nextLine(); | ||
Command command = ui.parseCommand(line); | ||
ui.lineBreak(); | ||
switch (command.name) { |
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 use of switch case to list all possible outcome, adding a default may be better
src/main/java/Storage.java
Outdated
} | ||
} | ||
|
||
public void saveTasks() throws IOException { |
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.
saveTasks maybe change to saveTasksToFile to make it more specific
src/main/java/TaskList.java
Outdated
System.out.println("Looks like you have " + this.list.size() + " tasks in total."); | ||
} | ||
|
||
void taskDone(int taskNo) { |
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.
we are missing a verb before taskDone, maybe makeTaskDone?
src/main/java/Duke.java
Outdated
public void run() { | ||
try { | ||
ui.intro(); | ||
Scanner input = new Scanner(System.in); |
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.
input is not a good name for Scanner, maybe sc?
Add assertions
Improve Code Quality
No description provided.