-
Notifications
You must be signed in to change notification settings - Fork 461
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
[Goh Yijie Jonathan] ip #499
base: master
Are you sure you want to change the base?
Conversation
src/main/java/Deadline.java
Outdated
* @param s Command containing task message and time | ||
* @param isCompleted whether the task is completed | ||
*/ | ||
public Deadline (String s , boolean isCompleted) { |
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 whitespace be removed?
src/main/java/Duke.java
Outdated
@@ -1,10 +1,51 @@ | |||
/** | |||
* Main java class to create a new Duke object |
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.
according to the module's coding standard, javadoc comments should end with a fullstop. This is different from git subject headers which states not to end with a fullstop.
src/main/java/Parser.java
Outdated
} | ||
if (taskAdded) { | ||
Task addedTask = tasks.get(tasks.getSize() - 1); | ||
System.out.println("Got it. I've added this task: \n" + addedTask.toString() + |
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 the +
would be nicer at the start of the next line:
System.out.println("Got it. I've added this task: \n" + addedTask.toString()
+ "\nNow you have " + tasks.getSize() + " tasks in the list.");
src/main/java/Parser.java
Outdated
public class Parser { | ||
private Storage s; | ||
private Ui ui; | ||
private TaskList tasks; |
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 better variable would be taskList
as tasks
might imply a collection of task
src/main/java/Event.java
Outdated
@@ -0,0 +1,34 @@ | |||
/** | |||
* Represents an Event that can be created by the user | |||
*/ |
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 more readable just before the class
src/main/java/Storage.java
Outdated
if (taskType.equals("T")) { | ||
tasks.add(new Todo("todo " + taskMessage, isTaskCompleted)); | ||
} | ||
else if (taskType.equals("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.
perhaps to you could start the else clause to the curly bracket to meet the coding standard?
if (condition) {
statements;
} else if (condition) {
statements;
} else {
statements;
}
src/main/java/TaskList.java
Outdated
} | ||
|
||
/** | ||
* functional method to delete a 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.
I believe the first letter in each javadoc comment should be capitalised?
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 not much issues. Your code is easy to read and I could only spot some minor coding standard violations.
There is a need to check that methods in TaskList and Storage class are being passed valid arguments Let's use assertions to implement these safety checks Using assertions will help to reduce bugs in the code and increase confidence in the correctness of the code
Parse method is too long and hard to read Let's refactor the parse method This will help to make the code more readable and scalable
Use Assert feature There is a need to check that methods in TaskList and Storage class are being passed valid arguments Let's use assertions to implement these safety checks Using assertions will help to reduce bugs in the code and increase confidence in the correctness of the code
* master: Use Assert feature
Improve code quality of code in parser class Parse method is too long and hard to read Let's refactor the parse method This will help to make the code more readable and scalable
Project Duke
Some of the core features include:
list
Completed taskHere is some random code: