-
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
[Saketh] iP #484
base: master
Are you sure you want to change the base?
[Saketh] iP #484
Conversation
Merged Level-7
Committing the javadoc branch
Merging the Level 9 branch
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, just a few nitpicks
src/main/java/duke/Parser.java
Outdated
s.appendListToFile(t); | ||
} else if (input.startsWith("find")) { | ||
String keyword = input.substring(5); | ||
ArrayList<Task> output = new ArrayList<>(); |
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 usage of plural form for output will work better?
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 the variable name should be completely renamed. The queried tasks aren't an "output"; they are the result of a query, and are then reformatted into a text result that gets printed.
src/main/java/Duke.java
Outdated
private Storage storage; | ||
private TaskList tasks; | ||
private File | ||
file; |
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.
Any reason why you put file on the next line?
src/main/java/Duke.java
Outdated
@@ -1,10 +1,53 @@ | |||
import duke.*; |
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.
Would it be better to list all the imported classes?
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 concur with @gordonlzy. I think this violates one of the coding standards.
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 agree as well. Imports should be one at a time.
src/main/java/duke/Parser.java
Outdated
* @param s The Storage that handles the reading and writing to a text file | ||
* @param file The file that gets written to and read from | ||
* | ||
* @return void |
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 it necessary to add @return for void?
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, but just a few nits to fix, that's all.
src/main/java/Duke.java
Outdated
@@ -1,10 +1,53 @@ | |||
import duke.*; |
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 concur with @gordonlzy. I think this violates one of the coding standards.
src/main/java/duke/Parser.java
Outdated
* Method that adds a Deadline task onto TaskList | ||
* | ||
* @param input The string that contains the user input | ||
* @param t The TaskList that contains the tasks to be added |
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 naming of some of the parameters could be more verbose; however this is just nitpicky, since your javadocs already explain what the parameters mean. Good work! 👍🏻
src/main/java/duke/TaskList.java
Outdated
* @param date The date mentioned in the task initialization. | ||
* | ||
* | ||
* @return void |
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 necessary for a void
method.
Merge add-gradle-support
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:)!! Great work overall!
src/main/java/duke/Deadline.java
Outdated
import java.time.format.DateTimeFormatter; | ||
|
||
public class Deadline extends Task { | ||
protected LocalDate by; |
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.
In my opinion, deadline seems to be a better name compared to by
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.
Quite good overall, but many parts could be improved
class NullTaskError extends DukeException { | ||
public NullTaskError() { | ||
super("OOPS!!! The description of a todo cannot be empty."); | ||
} |
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 it would be better for type to be in the constructor instead of the getMsg method.
src/main/java/Duke.java
Outdated
@@ -1,10 +1,53 @@ | |||
import duke.*; |
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 agree as well. Imports should be one at a time.
src/main/java/Duke.java
Outdated
private TaskList tasks; | ||
private File | ||
file; | ||
private Parser p; |
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.
Avoid one letter names; parser
would be a better name
src/main/java/duke/Parser.java
Outdated
String year = breakingDate[2]; | ||
String month = breakingDate[1]; | ||
String currentDate = breakingDate[0]; | ||
int i = Integer.parseInt(currentDate); | ||
if (i < 10) { | ||
currentDate = "0" + currentDate; | ||
} | ||
String finalDateFormat = year + "-" + month + "-" + currentDate; | ||
LocalDate date1 = LocalDate.parse(finalDateFormat); |
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.
Can be simplified using method LocalDate.of
src/main/java/duke/Ui.java
Outdated
|
||
public class Ui { | ||
Scanner scan; | ||
private String str; |
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.
Could be more descriptive; what String is str holding?
The reading from file method does not check whether the current task is done or not This needs to change so that the done nature of the task can be viewed by user The method of string manipulation has been updated to include this change Now every time the user asks for the list, the tasks that are marked done will be reflected as such in the output
Added assertions to the code This ensures that there are no unexpected errors in the code during running It is being fixed by adding relevant asserts statements in methods where applicable
Finish A-Assertions
Finish-A-CodeQuality
Duke
Your own personal task recording system
DukePro frees your mind of having to remember things you need to do. It's,
FASTEXTREMELY FAST to useIn order to access it:
And it is FREE!
Features:
Managing tasks
Managing deadlines (coming soon)
Reminders (coming soon)
This is a sample task list that you can view if you type the
list
command:[D][ ] return book by: 2019-12-02
[E][ ] meeting at: 2015-10-15
[E][X] project meeting at: 2015-10-15
Here is the main method of the application: