iP review comments dashboard

Agree

what if taskName is an empty String ?

Huge assumption, might tell the UI to print like "no deadline given, midnight is set to be the deadline time"

might give StringBuilder a function to handle adding line?

give to DukeException?

give to DukeException?

use i + 1 instead j make it looks cleaner

can make it private final ?

what if a task that has been done, is invoked with markAsDone again? exception

might name it addTask

might name it getTask

might name it removeTask, and what if index > 0 or index > tasks.size() ?

prevent using wildcards for importing packages

"The done index is missing."

give to DukeException might be better

give comment on why id - 1

what if index out of bound ?

why have 2 attributes for the state of task object?

Maybe can give comments? A little difficult to understand the code

try to avoid returning null

rename it into getTasks might be better

rename it into addTask might be better

rename into deleteTask might be better

Maybe can let UI handle the String format

why there is a newline?

personally i handle all printing with ui, but it's up to you

can add comment so people understand why index reduced by one

i used final modifier for my attributes, but I think it's totally fine

what if the task has done? throw exception

what is \u2713 and \u2718 ?

prevent using wildcards for importing

delete commands

Maybe can let UI handle the printing?

minor, but what if the list is empty ?

what if a done task is invoked with markAsDone again? exception

magic string can be set as static variable

as the textbook mentioned, avoid arrowhead style code

why limit the size to 100?

can use task instead of t for readability

avoid using null, can set it as an empty arraylist

Great job! I like it 😃

For better quality, should make the boolean isExit to be private attributes of Duke class

Should not declare it to be empty string here

This is also not a good practice

Should explain what happens here.

Missing curly bracket

What is parts[0]? Abstract more

e.g. String command = parts[0];

if (command.equals("T")) {

...

}

Also missing curly bracket

Curly bracket

Abstract more. What is parts[1]?

After reading this part, only then will I understand that | is the delimiter that you put to differentiate each component of the command. Should explain it clearly in the comments

Should use UI class to print

Same. Use UI class to print instead of doing it directly here.

print using the UI class

Again, print using the UI class

Maybe we can separate LocalDate and LocalTime to make it better

Curly bracket

Create other class named CommandsEnum and put this there

variable name could be better

Name is not descriptive enough. Should be Parser parser = new ...

Same. Should be Command command = ...

Looks a bit long, but barely below the soft limit, 108 chars. Place line break in order to be more readable

Deadline deadline = new ...

This one is also barely below soft limit of characters. Place line break to improve readability

Event event = ...

Name not descriptive enough. Should be Pattern pattern = ...

Name not descriptive enough. Should be Task task ...

Name not descriptive enough. Should be Matcher matcher ...

Name not descriptive enough. Should be ToDo todo = ...

Name not descriptive enough. Should be Event event = ...

Name not descriptive enough. Should be Deadline deadline = ...

Same here.

More descriptive variable name (e.g. convertedTaskList)

Name more descriptive

Although all the three classes in task package are used, perhaps listing them out explicitly will comply with the coding standard better? 🤔

According to our coding standard, perhaps there should not be spaces before each "case"? 🤔

Perhaps the naming here could be even more straightforward? e.g. formatInFile.

Good naming for method!

If it is a "TaskList", perhaps can rename the variable name to "tasks"? 🤔 (same applies to the rest methods)

Perhaps more Javadoc comments could be included? 🤔

Perhaps there should be a space between "Parser()" and "{"? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there are more spaces than usual? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space after "if"? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space after "if"? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space after "if"? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there is a missing space? 🤔

Perhaps there are missing spaces from this line onwards? 🤔

Perhaps there are missing space from this line onwards? 🤔

Perhaps some Javadoc could be added here for the public methods? 🤔

Instead of "taskLst", would it be better if it is named "tasks"? 🤔

Instead of "lst", would it be better if it is named "tasks"? 🤔

Perhaps constant variable naming could be applied here? 🤔

Good use of functional programming!

Perhaps can use the naming "tasks"? 🤔

Good use of functional programming!

Method header comments should start in the form "Reads"

https://se-education.org/guides/conventions/java/intermediate.html#comments

Method header comments should start in the form "Writes"

https://se-education.org/guides/conventions/java/intermediate.html#comments

Method header comments should start in the form "Creates a new ..."

https://se-education.org/guides/conventions/java/intermediate.html#comments

Method header comments should start in the form "Outputs a ..."

https://se-education.org/guides/conventions/java/intermediate.html#comments

May want to structure it like "Returns user input from standard input"

https://se-education.org/guides/conventions/java/intermediate.html#comments

This line conforms to https://se-education.org/guides/conventions/java/intermediate.html#comments format, but personally I feel that "Returns" would be a better word?

This line conforms to https://se-education.org/guides/conventions/java/intermediate.html#comments format, but personally I feel that "Returns" would be a better word?

Additionally, it should end with a full stop.

This line conforms to https://se-education.org/guides/conventions/java/intermediate.html#comments format, but personally I feel that "Returns" would be a better word?

"Deserializes string into a DeadlineTask"

https://se-education.org/guides/conventions/java/intermediate.html#comments

"Deserializes string into a EventTask."

https://se-education.org/guides/conventions/java/intermediate.html#comments

"Serializes task into a String."

https://se-education.org/guides/conventions/java/intermediate.html#comments

"Marks task as ..."

https://se-education.org/guides/conventions/java/intermediate.html#comments

"Serializes tasklist ..."

https://se-education.org/guides/conventions/java/intermediate.html#comments

"Deserilalizes ..."

https://se-education.org/guides/conventions/java/intermediate.html#comments

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

Format looks good to me, but perhaps if you have time, you could write a short description of the test cases?

"Avoid magic numbers" https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Might want to use a named constant for this?

"Avoid magic numbers" https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Might want to use a named constant for this?

"Avoid magic numbers" https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Might want to use a named constant for this?

"Avoid magic numbers" https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Might want to use a named constant for this? Might want to use this style: https://github.com/se-edu/addressbook-level2/blob/master/src/seedu/addressbook/ui/TextUi.java#L25

Might want to print an error message rather than printing the stacktrace.

Not sure if this is a code quality issue but perhaps you could return a String so that the Ui can deal with the printing to display?

If I remember correctly, this would be part of OOP's SOLID principle: https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/principles.html#solid-principles

Currently your TaskList handles the logic and the printing which violates SRP.

Should have a space between , and DateValidation

Nice! I didn't know you could use this to test the output stream!

I am not sure if this is a code quality issue, however, I would recommend using this.task rather than using this.getTask(). This is because this.getTask() does the same thing as this.task.

Perhaps if your getTask method does extra processing for the String that is returned back then it would be justifiable?

May want to use a static variable for "MMM dd yyyy"

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

May want to use a static variable for "MMM dd yyyy"

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Perhaps there is a typo for a variable name for deadline rather than deadlline.

importing tasks individually can be clearer.

Perhaps having a method name that briefly describes what it does would be clearer.

Not sure if I liked that the - does not have whitespace before and after it.

I would personally an access modifier for my method to show whether it can be used outside the package or class.

Perhaps instead of lst, using tasks could be clearer to know what the list contains.

perhaps rather than inputArray, you can have a plural form of the input to show that it has multiple parts.

perhaps extracting all the code that deals with different types of tasks could make your code clearer.

I like the name of this boolean, clearly tells me what it does.

Perhaps FXMLLoader should be camel-cased to FxmlLoader

Perhaps the variable name can be more descriptive than ap, so that it will be easier to know what it is down the code.

perhaps using plural form will be clearer.

I like this variable name, it is clear what it is.

perhaps t can be more descriptive.

I like the usage of newTask, it makes it clear that this is not in the tasks.

i like targetTasks, easy to understand that its a lot of targetted tasks.

Personally, I would use tasks rather than taskList, as taskList contains multiple tasks.

Perhaps it would be good to describe what 1 meant by introducing a constant, maybe DONE_MIN_ARGUMENTS.

Perhaps calling it inputs could be clearer since you are splitting the input.

I would personally extract the individual cases codes to different methods such that it will be clearer for readers.

Perhaps declaring what each index of the taskTypeSplit is could be clearer for readers.

Perhaps declaring what taskString[0] is could make it clearer for readers what it is being compared to.

Do you think that extracting the codes for converting the data file to its tasks could be clearer for the readers?

Perhaps extracting the data representation to specific task classes could make your code clearer.

Perhaps a more descriptive variable instead of d can tell readers what the input is?

I like this form of splitting the statement!

Perhaps instead of done, a variable name for the boolean could be better as isDone?

Perhaps showing what you are checking for in each case could be clearer?

Maybe rather than t, using task could be a more descriptive name?

Perhaps use a camelCase variable name and a more descriptive name rather than UC here?

Should this import statement be listed explicitly instead?

Same issue as above

Perhaps this instance variable should be declared final instead? (as your variable name is already in all uppercase letters)

Like the use of assertion here 😄

Should there be a space between Exception and the opening bracket {?

Should there be a space between DukeException and the opening bracket {?

I noticed the same issue in several other places too.

Perhaps you can remove the unnecessary this here?

Should this method be named differently because markDone does not really tell us that a task will be returned?

Perhaps remove the redundant this here?

Should the name representing packages be in all lowercase instead?

Should there be an empty line between the description and parameter sections?

Should the breaks be before the operator instead? (Should the + be on the next line instead?)

Perhaps the method name should be more descriptive of the action? (such as printSeparatorLine)

Should this import statement be listed explicitly instead?

I noticed the same issue in several other places as well.

Perhaps the break should be after the comma instead?

I noticed the same issue in several other places too.

Should msg be a standard word instead of a "texting-style" spelling as it is part of the javadoc and is meant for users to read?

I noticed several other places with this issue too.

Perhaps try to avoid deep nesting and code duplication here by instead using an outer try catch block that catches the same DukeException that each if clause throws?

Perhaps use a more descriptive name other than tk?

I noticed several other places using this variable name as well.

Should there be a description for the return value here?

Perhaps avoid using a magic number and use a named constant for comparison here?

Perhaps make the code more obvious here by adding parentheses around the same grouping?

Perhaps remove the redundant this?

I have noticed several places with this issue too.

I like how you make the happy path prominent instead of redundant else statements 👍

Perhaps use a constant here instead of a magic number 4 to make it clear what it means?

Similarly, perhaps you can use a constant here instead of a magic number 6?

Should workingDirPath be in SCREAMING_SNAKE case?

Perhaps a more descriptive variable name for mapper to describe what it maps and what it maps to?

Should divider be in SCREAMING_SNAKE case instead?

Should there be an empty line between description and parameter section?

Should there be an empty line between description and parameter section?

Is this additional blank line necessary?

Perhaps write a descriptive header comments for this method?

I agree with @jlxw48

Perhaps write a descriptive header comments for this method?

Perhaps write a descriptive header comments for this method?

Perhaps write a descriptive header comments for this method?

Perhaps it could be named as displayList?

Should there be an empty line between description and parameter section?

I agree with @jlxw48, perhaps you could further abstract this method?

Should there be an empty line between description and parameter section?

I think it doesn't really format text per se. Maybe naming it something like printSeparator() would be better?

I think this method should be named to sound like a boolean?

Should there be an empty line between description and parameter section?

Should there be an empty line between description and parameter section?

Should there be an empty line between description and parameter section?

Should there be an empty line between description and parameter section?

Should @param and @return be included?

Is it too descriptive? Perhaps have it more concise?

I noticed some methods did not have Javadoc comments. Perhaps, you could add them in?

I agree with @georgepwhuang. Perhaps you could include them in?

I think the @return is missing?

Did you forget to include the @param and @return?

I noticed some methods in multiple files that followed the same Javadoc format. Should there be an empty line between description and parameter section?

Perhaps can name it like markComplete?

I think the method name should be in a verb form?

I agree, should it be converted into a verb form?

Perhaps deadline in the constructor parameter be written as "deadline" instead of deadLine.

Perhaps h_rule can be renamed as HORIZONTAL_LINE_RULE. Perhaps it can be made a constant and hence named in all caps.

Should raw_in be named as rawIn or rawInput instead?

As mentioned by DrWala, perhaps "seq" can be renamed. What does "seq" stand for over here?

Perhaps a gap between the statement seq++ and the if statement would make it easier to read. I noticed this elsewhere too.

Perhaps "seq2" here can be renamed to secondSequence as it would make it easier to distinguish from the above statements.

As mentioned by DrWala, I think case statements should not be indented, but rather inline with the switch statement. Please check again. There's a setting in the IDE to make it easier for you to auto format in the future too!

Should "res" be named as "result" over here? It might make it easier to read.

Would it be better to have a gap between the scanner statement and the while loop statement? It might make it easier to read. I noticed this in a few other places too.

Should the * import can be replaced with the individual required imports? I think it was in the coding standard guidelines.

Should the * import can be replaced with the individual required imports? I think it was in the coding standard guidelines.

Should the extra lines between the statements and the method braces exist? It might make the code a little harder to read.

As above, perhaps some of the extra lines between statements can be removed and the statements can be grouped instead. What do you think?

Would it be better to rename isExit to hasExited or something else? I'm not too sure what it refers to.

Perhaps the code here can be replaced with a switch statement to improve readability. What do you think?

Should there be an extra line here? Noticed it in a few places.

I've noticed these lines being repeated in a few of the executeXXXXXCommand methods. Perhaps it would be better to abstract them out to make the overall code cleaner.

Perhaps these bunch of lines can be abstracted out too as well. They appear repeated in the other executeXXXXCommand methods as mentioned above.

Should there be an extra line before this line?

Perhaps the method name "save" could be renamed to "saveTaskList" or "saveToHarddisk".

Similar to the previous comment, perhaps "load" can be renamed to "loadFromHarddisk" or "loadTasklist".

Should this extra line be removed?

Should the * here be replaced with explicit import statements? I believe it's part of the coding standard.

Would it be better to rename "lst" to "list" here? Might be more understandable in the areas where it is used.

Since these enums are constants, should they be in upper case?

Would it be better to rename "f" as "file" to make it more readable?

Perhaps it would be better to rename AL to arrayList or taskList here.

Should this public method have a Javadoc comment above it? 🤔

Should this public toString() method have a Javadoc comment above it? 🤔

Should this public toString() method have a Javadoc comment above it? 🤔

Why is there an extra newline here? 😮

Should this public toString() method have a Javadoc comment above it? 🤔

Should this public toString() method have a Javadoc comment above it? 🤔

I like how you created tests for 4 separate classes 😄

Should these variables be capitalized, since they are constants?

Should this variable be capitalized, since it is a constant? 🤔

Should these methods have Javadoc comments above them, since they are public methods? 🤔

I like how you made the checkboxes constants 😄

Should this variable be private (with a separate getter method to retrieve this field in the class)? 🤔

Should this variable be private (with a separate getter method to retrieve this field in the class)? 🤔

Should there be a space after the word 'Task'? 🤔

I like how you separated the imports from different packages with a newline! 😄

I like how you renamed Duke with another name 😄

Should the extra space after 'storage' be removed? 🤔

Should the main method have a Javadoc comment above it, since it is a public method? 🤔

Should this variable be private (with a separate getter method to retrieve this field in the class)? 🤔

I like how named your method in a manner that explains exactly what it does 😄

Should this public method have a Javadoc comment above it? 🤔

Should this public toString() method have a Javadoc comment above it? 🤔

Should these variables be private (with getter methods to retrieve these fields in the class)? 🤔

Oh wow this is awesome! I like how you coded this yourself 😄

I like how you named your methods as verbs 😄

I like how these names are not too long and not too short either 😄

Should this be protected or private?

Perhaps you can consider extracting all these into their individual Command class?

Should this be protected or private?

Maybe you can consider converting this into a utility class whereby all the parse functions are static?

Isn't this ui unused?

Should this be protected or private?

Perhaps you can just pass in the ui instance you already created in Duke?

Should this be protected or private?

Similar to Parser class, maybe you can consider converting this to a utility class with static functions?

Maybe you can consider using the faster System.out.printf with formatted string?

Should the test be just calling the function? Maybe you can call functions that will invoke the ui showing those messages, then see if your ui actuall respond.

Perhaps can keep it as LocalDateTime to be more flexible, instead of having to parse every time

Consider removing this extra line break

Consider changing it to "Deadline" as class name should be singular noun

Consider removing these empty lines after the JavaDoc comments

Should it be private or protected?

Consider changing it to "Event" as class name should be singular noun

Consider removing these extra empty lines

Consider adding indentations and removing the empty line below

Consider isolating the execution of different commands into their individual Command classes

Consider placing TaskList instance outside of the Parser as parsing tend to only involve the translation process from a string into a specific object, in this case, a Task. The collection of tasks should be external.

Maybe the conversion of File to Task can be done within the Storage class, so all TaskList has to deal with is Task

Consider deleting this line as the empty constructor is implemented by default

Consider adding space between toString() and {

Consider removing the extra spacing between "Deadlines" and "deadline"

Can make it public I think?

Imported classes should always be listed explicitly according to Java coding standard.

May combine with the next catch block I think.

There should spaces around operators. e.g. tasks.get(itemNo - 1).isCompleted = true;

Remember to use a blank line to separate different parts, constructors, and methods according to Java coding standard.

Put a blank line to separate constructor from the private fields according to Java coding standard. And I think making the constructor public is better?

Import classes explicitly according to Java coding standard.

Since there are some many different types of exceptions the method may throw, maybe it is better to create a parent class DukeException of all of them and the method header may be shorter. e.g. public static int dukeRunner(String log) throws DukeException {

The body of each case part is so long. I think you may put them into methods which may increase readability of your code.

According to Java coding standard, is it better to make it private here?

Remember to use a blank line to separate different methods, constructors and parts of your code.

Perhaps using switch case rather than if else is more lightweight and readable here?

According to Java coding standard, always import classes explicitly.

Are there any reasons for not making it private?

Pay attention to the indentation here.

Can make the Task class abstract I think.

Are the blank lines here necessary?

Why don't you use the full name parser as the variable name? The same for others.

Why do you put a blank line here?

Each case block is too long here. Perhaps abstract part of them into methods with reasonable names which increases readability of your code.

Remember to always import classes explicitly according to Java coding standard.

Perhaps abstracting part of the code into methods with reasonable names is better in terms of readability?

If there is an empty block, is it better to comment on it to help others read your code?

Perhaps using blank lines to separate different parts of your code is better?

You may put a break for the default block to facilitate reading.

I like the use of packages for your project

Perhaps you could consider leaving an empty space between imports from different packages?

Perhaps you could follow the format below for a ternary operator:

date += i == commandSeparate.length - 1

        ? commandSeparate[i]

        : commandSeparate[i] + " "

Perhaps you could consider naming it differently since checkIfExist() sounds like a boolean expression, however, it performs a stateful operation instead

Perhaps you could consider adding a description on what gets returned and what triggers the Exception to be thrown?

Perhaps you could consider a different name for the Scanner variable?

Maybe you could consider a different variable name? Since the variable name of time could mislead another developer to think this is of the LocalDateTime type.

Perhaps you could consider adding the private access modifier to your variables?

Perhaps you could consider naming the variable name in camelCase instead of PascalCase?

Perhaps you could consider adding JavaDoc comments instead for public methods?

Perhaps you could consider naming this to sound more like a boolean?

Perhaps you could consider adding test cases where you set the index to be out of range as well?? Maybe something like done 200, delete 500?

I like that you broke the long String up into several lines

I like the abstraction of silentAdd() to add your tasks instead of directly adding into the list.

Perhaps you could consider a more descriptive variable name?

I like that you split a long String into two lines with the correct indentation.

Perhaps you could consider an alternative name to this method? Since getResponse sounds like a stateless getter method, other developers would probably not have expected a stateful operation to be performed.

Perhaps you could consider the use of a switch statement here?

Perhaps you could consider adding JavaDoc comments to describe what this public method does?

I like the naming of this boolean variable.

Perhaps the use of constants such as TODO_TASK_INDEX instead of "magic numbers" would be better?

Perhaps you could consider a different method name here since checkifTaskDone sounds like a boolean method, however, it performs stateful operations

Perhaps instead of accessing a task attribute directly, you might want to consider using a setter method such as task.setTaskAsDone() to perform this operation

I like the naming of this test method.

Perhaps you could consider adding test cases designed to throw exceptions, such as done 200 or delete 100 as well

Might have meant date instead of dead?

Perhaps it would be better to just let the child classes that require this variable implement it instead? If not it would be slightly redundant for the other classes to contain it.

Might want to declare what the value 4 means? If not will be classified as magic number.

Can leave a space between catch (.

Might want to consider implementing an enumeration for the type?

Might not be a good idea to return null; could potentially result in NullPointerExceptions somewhere down the line.

Might want to leave space between while (true)

Could consider using switch statement instead to make the code look cleaner!

Might want to use curly braces instead of leaving inline if statements

Might have exceeded line limit

Might want to include spacing between operators and operands

Consider leaving 8 spaces of indentation for line breaks

Should this be canTerminate? Might be better to change to reflect boolean variable name

Might have exceeded line limit

Might have exceeded line limit

Might want to consider shifting "+" operator to begin on new line

Might have exceeded line limit

Might want to consider removing extra space(s) between new Ui()

Good use of assertions!

Might have exceeded line limit

Might want to include spacing in between try {

Might have exceeded line limit

Clear and concise naming of method

Might have exceeded line limit

Should the indent be 8 spaces instead of 9?

Should this be broken up? It may be a bit too complicated

Should there be a space between the description and params? Seems to occur a lot, but only a minor cosmetic issue

Should the appends be broken up? Or perhaps in a newline eg

str.append(t.getSaveString())

        .append("\n");

Similar as below.

Should there be a javadoc for this method?

Should there be a seperation here? All belong to the vergil package so in my opinion they should be together

Should there be a default clause to the switch case?

Should these fields be final? They are not getting modified (and in fact if they do you might run into issues!) so no harm letting the compiler check for us?

Should the tests be in this structure?

Should there be javadoc comments for these methods?

Should there be a javadoc explaining the class? I notice most of your classes are missing this

Should this be listed explicitly? You can change your IntelliJ preferences to do so!

Should there be a header comment for this? Same with a few other classes too.

Should the + be separated by spaces? A few other instances of this around too.

Not a coding standard violation, but this is a cyclic dependency, perhaps there's another way to do this? (Not that this is bad but just a comment!)

Should there be a javadoc for this? Then the other commands can inherit the documentation too!

Should there be a space before the {?

Should the && be surrounded by whitespace? Few more instances in this class with operators

Should the else if be on the same line as the close brace? Occurs a few times below too

Should the acronym ISO be lowercase (formatDateIso)?

Should the variables be private? Getters can be written instead if necessary

I like your extended javadocs!

Should there be a singular space before { rather than 2?

Should tasks be private? It's not used outside of TaskList either

hmm... Is this necessary??? Or consider using a better variable name??

Consider using a better variable name?

Consider renaming this method name?? Is there any reasoning for it being run2(...). Else, u should use consider renaming?

exec as a variable name, in my opinion, is rather unclear?? maybe it should be a name that can be associated with parser class. e.g. dukeParser haha

i noticed that short hand as a variable name was used in most of ur Task subclasses. i feel like you can be clearer on that part even though they are being used in the same class. Just a suggestion 😃

You should add a blank line before describing your method headers.

I think u might have accidentally added a blank line.

Avoid using abbreviations in ur method names?

consider renaming the name that is more explicit and applicable to the program

U should add a blank line after describing ur method and before the start of describing method headers.

Perhaps, a better variable name can be used here instead of by to make it more explicit that its the date/time for the deadline.

There are spaces here in the javadocs. Could've been on accident but u should remove it to avoid the docs to look weird haha

inconsistent spacing in the method header comments. I think I saw this in other classes as well so please fix them when u can! 😃

Good on you for following the guide and adding a default block for good practice!!!

I think you should not write "args unused". Consider using the following description instead 😃

"@param args an array of command-line arguments for the application"

I am not sure if this is necessary or a mistake, but following what the description of the level A-MoreOOP said, consider intitalizing these objects in the Duke constructor so the main is just treated as a main entry point instead where u can just initialize the Duke object and call the run method.

I agree with enhao25! Variable names are important to convey to the reader what your code is actually doing and what the variable is for. Perhaps, consider renaming them to names that fit the context.

I think this one can be done better as both the if-blocks have common conditionns. The first condition has an OR condition so maybe that can be used to "differentiate" the exceptions thrown.

I agree with enhao25! Although its a very very small issue, it believe it gives off a better impression of your coding 😃

Perhaps re-consider the name of this variable to a more descriptive one haha

Perhaps consider making this as a constant that can be used with the final keyword and changing the variable name to "LINE_SPACING".

great use of the ternary operator to remove a redundant if-else here 😃 Definitely will use this in my project too for these nitty-gritty if-else conditions haha

Maybe u should group "junit" imports together

i am not sure if it's a glitch on github's part or ur IDE altered the spacings for ur javaDocs but do check it out i am sure this isnt intentional hahaha

toDocreationTest() should be followed by whitespace

same as above, whitespace is missing after ()

Maybe you can move the last + to the second line to better fit the coding standard?

perhaps for the array to represent a collection of details of this deadline, name descriptions in plural form is better?

agree:)

agree with the test method convention:)

i like how you separate two append in two lines👍

Here, it would be better to put + at the start of each line?

eventDate could be used instead of at maybe?

haha interesting greetings from Duke 👍

here and in other Command classes, is s is missing for the verbs

Maybe the comment should start with a verb to describe the method?

it would be better if you add method header comments for each method 😃

The naming of at and time could be clearer to make the loops more readable.

maybe the name of the ArrayList should be a plural noun i.e. tasks?

The naming of by and time could be clearer to make the loops more readable.

agree

is break really necessary here since it only contains one e? keeping the throw statement one line can make it look more concise?

+ should better be placed at the start of a line?

what if when the tasklist is empty, but user command list ? Instead of an error message, can a message be generated to indicate there is no task in the list currently?

the method name could be findTasks maybe? to make it more specific

interesting and useful method!

but the method name remind can be more specific to better explain the method action.

Also, codes in this method seem too 'clever' which is not recommended according to textbook, so maybe can separate the long-winded filters into different statements.

Perhaps dateD and dateE can be improved. e.g. deadlineDate and eventDate are clearer, more readable.

4 seems a magic number. Maybe you can add in comment to explain.

All classes and public methods need header comments, please check here: https://se-education.org/guides/conventions/java/intermediate.html#comments

You might have missed the space after "," in the input parameters

Should there be an empty line here after the comment?

This is a good JavaDoc example!

You mean returns the command to delete?

"Sets" instead of set?

Is this line not used?

Please use a verb for method names like "getSize"

Please remove space after the bracket, should be: ...(IOException err) ...

Please use 2 lines instead for long statements like you have done in the last test in DeadLineTest.java

I like how you check for invalid inputs!

Just curious, why do you choose to use StringBuilder here?

Good naming!

maybe "task" instead of "single" here?

Distinct import statements please 😃

According to https://se-education.org/guides/conventions/java/basic.html#comments, please state @return before @throws

Good one line logic here. Perhaps using "taskStatus" instead of "status" would make it clearer to the reader?

DF1 and DF2 can be more interpretable names. "inputFormat" and "outputFormat" maybe?

Another curious question: why LinkedList and not perhaps ArrayList?

Since the method only adds one Task at a time, "addTask" would be better than "addTasks"?

Did you have a purpose for this class?

I see, thanks for sharing!

I see where you're coming from now 👍

Oooh coding with foresight, that's great!

Should this be isDone

Should be a verb right?

Should this be in abstract class?

should this be named differently?

Shouldn't you have javadocs for the most important class (Duke)?

According to the 2103T guide, should the descriptions of your tags(@) start with a capital letter? This seems to be up for debate, as the oracle guide and the 2103T guide on javadocs differ.

Should your files be in a package so that you can use protected methods instead of public ones?

I like how you've abstracted the commands

Should you have a finally as best practice? This is fine too.

I like your whitespacing!

Should you be more specific? run() merely drives Duke, not the program as a whole.

Why did you choose to modify toString instead of creating a new method?

Is there a better-formatted way of doing this as the number of types increases?

why did you choose to use the name "tokens"?

I like how this is succinct!

Should you have done initialized this earlier so you don't need to keep repeating yourself?

Are these empty lines necessary? They don't seem to separate distinct logical blocks.

Should you split this into two lines since it is just shy of the character limit?

Why \u2713?

Should you be capitalizing "A" when the rest of your javadocs have lowercase for the first word of your tag descriptions?

I'm not sure if this is an issue with Github, but it's showing me that your indentation for this wrapped line is not 8 spaces

Should you be splitting some of this into multiple tests?

Same as above.

Comment indentation should be aligned to method code.

Pretty long method. Could be good to abstract out the logic for individual case clauses.

Should have an empty line between description and parameters.

May be better to return tasks here to avoid using the else clause at the bottom. This will avoid further nesting of code.

e.g.

if (myFile.createNewFile()) {

    System.out.println(...);

    return tasks

}



System.out.println("File already exists");

...

Could have line breaks in between to make it more readable for others. More info here.

Or put them into their own Command classes

Would be good if the Tasks themselves could parse the string to create back the instance, e.g.

if (type.contains("[T]")) {

    Task todo = ToDo.createFromStoredData(type);

} else if (type.contains("[D]") {

    Task todo = Deadline.createFromStoredData(type);

} else if ... {

    ...

}

So the code would be more readable 😃

No space after output. Should be output = Parser.parse(tasks, input);

JavaDoc not following style guide. Should start with a verb. I don't think this is the right comment for the method too. Could be Returns bot's response after processing user input

Should be Adds task ...

No need new line after JavaDoc

Need to have a space before the =, should be this.type = type

Need to have empty line between description and parameter section or else the JavaDoc won't be rendered properly when generating documentation.

Need to have space before and after -.

Need to have space before and after +

Need new line between description and parameter section

Description should start on the line after /**

Need to have spaces before and after condition, i.e. if (allTaskMsg.isEmpty()) {

Same for here

Constant names should be uppercase with underscores to separate words.

Perhaps use a more intuitive variable name here such as taskList?

Should the bracket and the word 'do' have a spacing between? I noticed this same issue in several other places too.

Should there be a spacing between characters in the bracket?

I like how you catch each exception separately, however the handling done for each of them appears to be the same function?

Perhaps this while loop could start on a new line?

Should wildcard imports be used?

Perhaps a spacing could be used after the comma in "parseTarget, String delimiter"? I noticed similar issues in other arguments separated by commas in round brackets.

Should the +'s be separated by white spaces before an inverted comma?

Should comments be indented in the same line as its code?

I like how you named the regex String as pattern 👍

Perhaps function names should follow camelCase?

I like how you have followed screaming snake case for constant names 👍

Should a more suitable variable name could be used here? Perhaps singular instead of plural?

Perhaps method names should be verbs? I like how it is already in camelCase?

Perhaps constants should be in screaming snake case? I noticed the same issue at other parts of your code

Perhaps a more intuitive variable name here?

I like how your variables are named intuitively 👍

Perhaps nesting if-else statements like this might be a little confusing? Maybe consider using a switch case?

Should the return result of tasks.getTaskList() be directly passed into writeTasksToFile()?

Perhaps it could be neater to save the variable and pass the variable into the second function, thus sticking to the same level of abstraction.

Maybe:

TaskList t = tasks.getTaskList();

storage.writeTasksToFile(t);

I like how this snippet of code is efficiently condensed into a single line. However, perhaps you could expand it into its parts of split, obtaining the value from the array, then applying .strip()?

I noticed this idea could be applied to other parts of your code too.

Perhaps a better name for this function would be printTasks()?

Perhaps extract out the statement within the parenthesis for easier reading?

Preferably java.* should be above any third-party packages

Please leave a line break between the description and parameter section.

Do you think it will be better to name the String as dataFilePath?

Will it be better if Integer.parseInt(description) - 1 is extracted as a variable first since it will be used multiple times?

This is very similar to the event case, will it be better to extract this as a function?

I like how you are using the default branch to catch errors.

The use of this is to be avoided according to the style guide.

Naming the variable as *Of* is rather awkward, do you think dataFile is simpler and straight to the point?

I think this should be lineNumber right? It is pretty misleading on the first look.

Do you mean HORIZONTAL_LINE?

Will it be better to have a default branch, since String can take on any arbitrary values and no checks are in place to ensure correctness.

Do you mean index of the task?

I see that this wraps around every reply, is it better extract this out to avoid multiple statements like this?

Extraneous semicolon!

Period is a confusing way to describe the number of remaining days, perhaps getDaysRemaining is a better choice?

I assume this method can be made abstract?

data is very vague and not descriptive of the content of this array, maybe entries?

It will be better if there is an empty line between description and parameter section.

Please place the line break before the + operator

I like that there are no wildcard imports

Accidentally left an extra blank line here?

Do you think that it is better to name it as setStatus?

Be careful of the spaces 😉

    public void writeTaskToFile(List<Task> tasks){

You might have inadvertently added an extra blank line here:

https://se-education.org/guides/conventions/java/intermediate.html#layout

The indentation is a little off here: should be in line with the block.

        try {

The catch keyword should be in line with the closing brace.

        } catch (FileNotFoundException e){

Another space missed 😉

        while (input.hasNextLine()) {

The case statement should be in line with the switch, and the case blocks one indentation lower.

            switch (s){

            case "todo":

                System.out.println("____________________________________");

Another missing space 😉

And another 😉

You might have missed this: variable names could do with a more specific name, especially since the scope isn't small. Perhaps s1 can be renamed as userInputLine.

Applies to a couple other lines below.

Not a coding standard, but figured I could chip in. Since the catch block is the same as that below in line 86, you can consider removing the try-except block here.

Note the spaces here as well:

                    for (int i = 1; i < myList.size() + 1; i++) {

Consider adding a couple of class-level comments describing the class.

Might not be immediately clear otherwise what this Database class is intended for.

Your import statements are very nicely formatted 🎉

You have a Ui object passed to the command, which ideally handles message passing to System.out.

Can consider offloading these to the ui instead.

Same as in the other commands.

This line here states that DukeException is thrown, but only TaskIndexOutOfBoundException is actually thrown.

This will require the stack to explicitly handle the general exception instead of a more specific one.

Consider changing it to the throws TaskIndexOutOfBoundException?

Same goes to the other commands.

I couldn't immediately tell what this error is, perhaps more so from the user perspective.

A more descriptive error message might help, e.g. "Argument missing! Event needs to be supplied a date.".

Should describe what the function is intended to achieve, instead of stating how.

Perhaps, "Passes instruction to main routine to exit the program"

It's not immediately clear how the datetime is parsed.

You might want to add in a description of how this parsing is done as well, as an

additional paragraph under the method summary.

You probably inadvertently missed out commenting this public class.

isExit

You may want to use relative path

You may want to abstract each case into a separate method

splitInputs?

DukeIdkException?

You may want to change the constant var name to be FILE_NAME and FOLDER_NAME.

I think relative file path is prefered

Redundant empty space between ( and !isExit?

You may want to change to eventDescriptions.

currentTasks and newTasks?

You may consider using an advanced for loop here.

You may want to change the var name to tasks

taskArray or simply tasks?

FORMAT_ONE and FORMAT_TWO?

PARSER?

STEVE?

parsedInputs?

parsedStrings?

TASK?

You may want to make them all uppercase

I think adding an empty line between these logical blocks would help readability

Seems a little odd that your package is called ssagit when the application is still called duke.

Maybe follow the javadoc style of comments? Use '@param' and '@return', etc.

This is really hard to read, why do you need to catch the same kind of exception in so many different places? Maybe there's a way to have only one try-catch block?

I think UI should be Ui (lowercase i)

I feel these exceptions should be under a different class, like a DukeException suggested in the project guide

Whew this is long, maybe try moving some logic to other methods.

This doesn't seem like good practice to me, should handle the todo and event in their respective cases I feel

Maybe consider moving the printing of error messages to be handled by the UI?

Extra blank line here

Maybe move "Will print out" to the next line

Add an empty line here?

Maybe use 'tasks' instead of list.

Could add empty line between description and parameters section, here and in other places

Would it be better to change 'count' to 'index'?

Could add an empty line here between the variables and constructor.

I think getNextLine or even just nextLine would be a better name, getLine implies getting a specific line from a line number

This method doesn't print the message, just returns it correct?

Sorry, I don't get what this does...

Maybe move 'Input given will not change' to the same line as 'how function gets executed.' Noticed it in some other places as well

Maybe commandsSplittedByWhiteSpace or just commandsSplitted might be a clearer name?

Maybe the plural variable name commands will be better for an array?

Perhaps a clearer name would be better, also maybe this string array could be placed in toString() since it is not needed anywhere else?

Maybe taskDisplayStr or taskDetailsStr might be clearer in showing this string's functionality?

Perhaps naming the methods with this format:

whatIsBeingTested_descriptionOfTestInputs_expectedOutcome

e.g., intDivision_zeroDivisor_exceptionThrown

would better show what is being tested.

Perhaps a plural name tasks would be better? Maybe the name "tList" could be potentially confusing as well when referred to further down or in other locations, as it could mean task list or todo list.

Perhaps the parameter name could be taskList instead of tasks so as to clearly differentiate between TaskList objects and Task objects or Tasks (list of Task objects).

Perhaps this name could be puzzling as readers might wonder what "old" means? Maybe a clearer name would be better.

Maybe a name that clearly explains the functionality of this counter would be better? (i.e. what is this counter for or what is it counting?)

Perhaps a plural could be used for the variable name of this string array?

Maybe end with a full stop for consistency for the description of classes and methods?

Maybe tasks will be a better name than taskList?

Perhaps isExit would be a better name?

I like that the name was capitalized to follow coding standards for constants. 😄

I like that constant names have been capitalised to follow coding standards and that Javadoc of class members have been specified in a single line. 😄

Perhaps a space can be placed between each section/kind of imported statement, to show ordering and segmentation better?

Perhaps todos might be a better name?

Maybe a plural variable name would be better for this list of messages?

For names of constants, maybe capitalizing and using underscores to separate words would be better?

Maybe it will be better to take away the space in front of case clauses?

I think ENUMS are better in their own class. Do you have any reason to put these here?

I think it is better to list out the imports rather than importing by *

Any reason about the usage if-else instead of switch? I think using cases will be nicer.

I think writing one lettered variable is not very descriptive. Perhaps you should try more descriptive names such as file and scanner

Similar point with the above reviewer, a javadoc would be nice here.

Perhaps it is better to make all error messages similar.

This line is pretty long. I think you can split this into more lines. I like the usage of one-liner if-else, though 😄

I recommend to rename this with isDone to be consistent with the coding standards.

Another long line. Also, I recommend using iteration form of for for( >T> i : list) for better formatting. This method is okay, too.

Agreed with the first reviewer. A javadoc to explain specific tasks is good.

I think the variable d here is not very descriptive. Please change it into a more descriptive name.

This is pretty long for my liking. Maybe you can consider splitting it into two lines.

These variable naming is rather confusing. Maybe change it into lists and writer?

Should this variable named more descriptively? This naming also appears in many other places.

I think you are reusing the arr variable over this piece of code. Maybe can introduce a new variable to make it more clear. This is applicable in many places on this piece.

I think its better to name the array resulted by the split() to reflect the delimiters.

Maybe rename this into byDate so that readers can understand this should be a date

This can be renamed to describe that the task here is a new deadline. This appears in many places in the code.

I think the use of * in import statements are not allowed. Please consider listing down everything instead of using *?

Maybe you want to try checkStyle? It will adjust the import order to the correct order. But anyway, this course does not specify the import order.

According to the coding standard, a line between description and parameters are required. Maybe an empty line here makes it more proper?

Edit: Same for every other javadoc.

In the coding standard, there is one line:

In method header comments, the first sentence should start in the form Returns ..., Sends ..., Adds ... (not Return or Returnning etc.)

Maybe you want to change every javadoc starts with verb?

I am not sure parameter s is compatible with our coding standard. But a meaningful naming will always make it better. What do you think?

addString sounds like a verb instead of a noun. This string sounds like a method instead of a variable.

Edit: okay after reading more of your code, I am thinking this naming might not be wrong.

Indentation should be inline with the switch according to our coding standard. I still don't know why they are doing that...

Missing something?

Missing a space here. Same for the next method

Maybe consider changing this main to testMain or something?

This method is too long. Maybe you want to abstract something out and cut it down a bit.

According to the code quality chapter in the textbook, a method should not get more than 30 lines.

You may want to check here: https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html

Consider extending {} to a new line?

Maybe splits instead of split?

This comment carries no information. Maybe delete it?

The comment should be in a separate line.

This is a good point! I am going to change my code!

lineGetter is a noun, and we should verb for the method name.

(But I personally also like to use this kind of name. This course has soooooo many strange rules.)

I suppose it should be trimWhiteSpaceTest_xxxx_xxxx if I am not wrong?

Perhaps you can phrase your JavaDoc comment in a different way, such as "Adds a Deadline object into the tasks after parsing of execution" and "Raises an exception ..."

Just a tiny nitpick, but consider adding a space between your parameters inside functions to improve readability.

I like how you're checking for invalid inputs with very specific exception messages! This way the user would have an easy time telling what's wrong.

Maybe you could name this boolean as isExit to make it more obvious that it's a boolean variable.

Just curious but is there any reason you used LinkedList over ArrayList?

I like the very specific class name.

Great line break to improve readability!

Perhaps consider importing specific packages instead of using the wildcard import.

Maybe you could consider using spaces in between the colon to make it easier to read?

Perhaps you could be clearer with the naming of this variable, maybe use dateBy instead of just by?

I agree with this, great suggestions!

Perhaps you could consider using a more specific name such as dateBy so it is clearer to readers?

Great indentation to improve readability!

Great practice of capitalising the constant variables. Just curious but is there a reason why you chose to use constant variables over enumerations?

Perhaps you could specify what this index is referring to and rename it as such. Maybe indexOfDescription?

This looks a little like arrowhead style code. Perhaps you can consider simplifying it so that it's less complicated and indented.

Maybe you can consider using Creates instead of Create

Perhaps you could leave this JavaDoc comment out, as it seems to be repeating the obvious.

Would variable name for boolean isDone be better?

Would containsKeyword be a more appropriate method name?

Perhaps joining the separators as a string before and after the msg and then printing the output would be better instead of calling System.out.println() multiple times?

Perhaps an empty constructor would be unnecessary to be included as a default constructor can be used instead?

Extra blankspace here

Perhaps you could standardize if you would like to add a blank line after declaring class or not? I have noticed inconsistencies of blank spaces after class declaration in several other places too.

Perhaps you could use LocalDate to store the deadline date for more flexibility

Perhaps the use of isSavedBefore would be more suitable as a boolean variable name?

As there is only one parse(str) method available in the class, perhaps the "this." is unnecessary?

Should remove unnecessary whitespace/blank lines

I noticed similar issue in several other places

For @return tag in javadoc, should include description on what the method is returning

Consider adding a space before the opening braces

Consider adding a space before the opening braces. I have noticed the same issue in several other places too.

Empty constructor is unnecessary.

Should take note to avoid adding additional/unnecessary whitespaces in your code, such as the double space in "Deadlines deadline = "

I like the name you have chosen for your chatbot

The coding standard says:

Write descriptive header comments for all public classes/methods.

Although I agree that there is nothing much to write about here, it would be nice to still have a JavaDoc comment for it 😄

Same as above, maybe it would be better to add a simple comment like Constructs a DukeException object.

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Header comment can be added here

Would it be better to use a more descriptive name for this variable?

Perhaps str can have a more descriptive name like taskDescription?

Same as above, maybe str can have a more descriptive name?

Perhaps this method can be renamed to markAsComplete?

According to the coding standard,

No blank line between the documentation block and the method/class

Shouldn't the import statements be arranged in lexicographic order?

I think it is better to store the horizontal lines as a constant as a final variable.

I think the empty lines should be deleted to make the code neater.

Should be in lexicographic order.

i think its better to have a line separation here

Naming of variable should be meaningful. Perhaps you could change to it to myTask instead, similar to the one above(ArrayList>Task> tt)?

Similar to the one above, it should be arranged in lexicographical order

i think the empty line here should be removed

I think there shouldn't be an empty line here

Should this be written as checker = 1 instead?

i think there should be an empty line between the description and the parameters

same for this, i think there should be an empty line.

Is the line a little too long? I think its better to split into 2 parts.

I think this empty line here should be removed to make the code neater.

Should the println statements be shifted into the UI file?

Should n could be changed to index to make the var name more meaningful?

Should the println statements be shifted into the UI file?

I think you may have forgotten to resolve the conflict here?

The conflict here seems to be unresolved too

Would getTaskCountMsg() be a better name to make it consistent with the other method's namings

Would it be better to implement the class as a concrete class instead? The intention of an interface is to act as a contract.

Maybe you would like to rename it to a more informative name, e.g. argumentsFromInput (might not be the best name, but I can't really think of other alternatives)

How about using the pair words key and value, since they will be used in a HashMap

Should this be @Test (looks different from the rest)

Not sure if you want to include the package name in the README.md title folder. The change is possibly due to Intellij's refactoring

The path currently is not valid, i.e. there's no file duke.Duke.java so you might want to change it.

I think it would be good if you added the Javadocs to the classes

Would it be better to indicate that the two index's here are of different types? I was a little confused as the index which is the class attribute was of int.

Maybe it would be better to specify something like indexString in order to distinguish the two.

Note: Similar for the other classes using index

I's assume Ack stands for acknowledge, but would be clearer to rename it to something like printTaskListStatus?

I think it would be more informative to name the string as errorMessage, so that it would be easier for others to understand the purpose of the string on first glance.

Comparing parseInputToDateTime and parseSaveToDateTime, Input is a noun in the first case, but Save is not a noun. Would it be more consistent if you rename it to SavedInput?

Would it be better to keep it consistent and name it displayGreeting instead?

Same for the ones over here. Would be good if you use a consistent verb (between print, display and show)

Maybe a better alternative for this method name is MarkAsDone? as names representing methods should be present tense verbs.

Do you think a good alternative for this variable name would be updatedTask?

Just a suggestion, but maybe a good practice would be to make tick and X symbols constants. for e.g. TICK_SYMBOL = "\u2713" and CROSS_SYMBOL = "\u2718" to avoid having magic strings in the code.

Instead of using .substring() with magic numbers, maybe you can consider using string.split(" ", 2)[1] to get the string after the first space.

I think it would be good to capitalize the first letter of a sentence for JavaDoc comments.

I like how you make delimiter a static var to avoid having magic string in the code! 👍

Do you think a better alternative for this method name would be isInDescription?

According to Java coding standard, Names representing methods must be verbs .... Maybe a good alternative for this method name would be getTaskInformation?

Maybe it would be good to use named constants for "+" and "-" to avoid the use of magic strings in code?

Maybe named constants can be used for command tyles and delimiters too to avoid having magic strings.

I like how you named messages for different scenarios, which improves the readability of the code.

I personally think this method can be slightly too long and has too many responsibilities. In order to follow SLAP closely, maybe you can consider extract the methods to check invalid inputs, perform the task, and print feedback for the task.

I like that you updated your project README!!

I like that you used a named constant for your delimiter to avoid the use of magic strings in the code. 👍

I personally think this method is slightly too long and has too many responsibilities. In order to follow SLAP closely, I think you can consider separate it into a few methods, including createFile(), readFromFile(), createTasksFromFile(), etc.

Maybe you can consider making all the messages / strings named constants at the beginning of your class, so that the meaning of each string is well-explained, and it makes your code more succinct and readable as well.

I like how easy it is to read your code! It is also good that you avoided deep nesting and having an arrow head structure. You did this practice earlier as well so good job!

It is good that your boolean variables are named to sound like booleans. I also noticed the same practice in several places such as boolean method names too. Keep it up!

It is good that you named collection of objects in plural form. You did this in several other places too. Well done!

It's good that your indentation of case clauses follows the java coding standard.

Your try-catch statements follows the java coding standards. You also did this for several other places too. Well done!

Your if-else statements follow the java coding standard. I noticed that you did this practice for several other places as well. Keep it up!

You followed the java coding standards for your package statement. You did this for other package statements as well. Nice!

Your imported classes are listed explicitly. You did this for other classes as well. Nice one!

I'm not sure if this is considered somewhat of a deep nesting, resulting in a arrow head structure?

I like how there is no deep nesting (arrowhead) here!

I like how you made the happy path prominent here!

I like how you your method name accurately explains to the reader of the method's functionality. Even if it may be long, it is clear.

Could this be done in steps? I'm not sure if I like this complicated expression as it makes it harder to read.

Could this be abstracted further to follow the SLAP?

I like how you split a long line into several short ones to enhance code readability.

Better to separate it into 2 lines

May be better to store it in a string variable.

There should be a space after "+"

searchDate may be a better name

Store the lambda inside a function?

you could just create an ArrayList at the start to remove repetition

Good effort in catering to all possible input. Perhaps what you can do is to just throw an exception if the input is not in the correct format

Perhaps can change the attributes to final since it is not reassigned

I think description would be a better name

Can just return the string directly without storing it in msg

Can just return the string directly without storing it in msg

Can remove the single line here

getCategory() would be a better name

Comments can be placed in the javadoc instead

Empty constructor is not required

maybe can try to put in return type of the function in javadocs?

maybe march the codes to the left margin?

maybe can add return type of the function into the javadoc? The prob exits in multiple parts of the PRs i think, if am not wrong

maybe should leave a line after this line?

maybe can try to use more discriptive words, instead of such short word, so that reader can understand what this variable repesents.. this is what i think

if i am not wrong. i remember its 4 space indentation for the codes in the function body?

should we remove such lines after resolving the merge conflict?

can remove this line. same for other parts of the code.

i think maybe can delete the blanklines between these import statements.

maybe can not more explanatary variable here,

which can better explain the purpose of variable

maybe can replace nested if-else with cases, and a default?

maybe can leave a blankline after this to group similar codes together for better readability?

would it be better to extract out this nested if-else statement,

as a method, so that the reader do not need to trace the code too much?

would it be better if a noun is used,

instead of verb. For example, can be taskAdder?

I'm not so sure for exceptions but class names should be nouns according to the coding standard.

Maybe DateTimeFormatException is better since it already implies that it is invalid/not recognised.

Since this is a constant, it should follow the coding standard of all uppercase and using underscore to separate words.

Instead of naming inputsAreValid, areInputsValid seem more inline for a boolean return value method.

I think 'at' is too short a name for a variable that has a rather large scope, should Event class become bigger in the future it might cause confusion in the future.

Since it might mean time or location.

Maybe you can add comments on what this class does?

Maybe you can explain that the command interface interacts with the input of the user and how the classes that implement this interface would have what methods in common.

Some inconsistent ordering of import statements compared to DeadlineCommand.java

Similar inconsistent import statements

Maybe updateListInFile is more appropriate and more similar to your other method readListFromFile.

I think its better to use numOfTask than plural form since most plural form should be used for some form of collections and not be confused between them.

It might be good to separate the imports based on packages so that there is consistency and is readable (coding standard)

Same thing about import statements, consistent and readable. (coding standard)

I think having a more verbose method name would help in identifying what it does exectly

If the class is not storing any variables, it might not be needed to initiate an instance of it, instead, you can just make all the methods static

The class itself looks fine, however I would suggest leaving an extra spacing after each '{' curly bracket openings.

It might look better without an additional newline after each try-catch / if-else statement.

The new line format used should perhaps be more consistent in the entire codebase.

The class itself looks fine, however I would suggest leaving an extra spacing after each '{' curly bracket openings.

The class itself looks fine, however I would suggest leaving an extra spacing after each '{' curly bracket openings.

Instead of using 'tdStr' to represent task description, maybe you can rename it as 'taskDescription' as it makes the variable more easy to understand.

Instead of using 'tatStr' to represent task at, maybe you can rename it as 'taskAt' as it makes the variable more easy to understand.

I like how the boolean 'isDone' really does sound like a boolean variable. It is easy to differentiate it from other type of variables.

Perhaps it would be better to rename tasks as TASKS since it is a final variable.

There is quite some content to go through when looking at these if-else statements. A suggestion would be to use switch cases, and perhaps do a single try-catch block that handles different kinds of exceptions.

The variable isDone sounds like a boolean return type instead of a String, perhaps it will be better to rename the variable.

It might sound better as a boolean variable if you rename fileExists variable to isFileExist.

Perhaps the variable names could be in upper cases, since they are final variables. It might be difficult for one to differentiate the constant variables.

Declaration of deleteIndex and count could be placed at the top of the deleteTask method, to easily identify where declared variables are located at.

Would you like to change the name of the parameter to a more meaningful one!!

Would you like to change the parameter name to a more meaningful one such as "commandInput" or "input"?

Your Javadoc is really detailed and compete! Good job!

Would you like to make isExit a member of the Command class, so that you don't have to override as a method, and can be initialised in constructor!

Would you want to change the if-else statement to the switch statement, so it is neater!

It is good that you changed the default switch statement format to the one that this module follows!

Will it be better if you change the parameter name s to a more meaningful word like "taskName"?

Would it be better if you make your class member private!!

Maybe it will be better if you name your parameter using a noun such as "date"!!

Maybe it will be better if you name your parameter using a noun such as "date"!!

Would it be better to put while loop inside the try block?

Maybe you wanna change the if -else statement to switch statement to look neater?

You may want your class member to be private!

It is good practice to check if the path exists before directing to the path!

Javadoc comments should start with caps 😃

e.g @param x X coordinate of position. Instead of @param x x coordinate of position.

Should not have a space between Javadoc comments and the method. 😃

Remember to have a spacing between the method name and the open curly bracket 😃

e.g finish() { instead of finish(){

Add a Javadoc comment for this since it is not very obvious what it does at first glance. 😃

The method name for this is not very clear, what does it mean by finish()? Perhaps change it to markTaskDone()? 😃

Wrong Javadoc comments, should include @return as well. 😃

try not to import with *, give specific imports! 😃

Remember to leave spacing between the name and the curly bracket! 😃

Agree! 😃

For this variable, I would suggest to give it a Javadoc comment because it is not very obvious what is it at first glance 😃

From the textbook -> Be wary when a method is longer than the computer screen, and take corrective action when it goes beyond 30 LOC (lines of code). The bigger the haystack, the harder it is to find a needle.

I would suggest trying not to put all the logic in here, perhaps can create new methods in this class to handle certain common things! 😃

This method is called 'print' but in fact it is adding certain new task to the task list. Perhaps you can try to refactor this method to make it cleaner! 😃

Agree! 😃

Perhaps you can change this method name to findMatchingItems because findItem makes it sound like you are finding 1 specific item but the return type is actually a tasklist 😃

Strictly speaking, I remember each sentence should end properly with a full stop.

I am pretty sure you can access your private fields instead of going through iceBear.parser. However, I am unsure which one you should go with.

What is the difference between this two methods? I think it would be good to include some java doc comments or make the method names distinct.

A switch case should be apt here.

I am not sure why you are using java.io.IOException instead of just IOException.

Is this necessary? What if users choose to input dates in another format? So, I would think it is better to make the user input a certain format like yyyy-mm-dd instead of catering to the different formats that a user can input.

I think you can use the inbuilt methods to achieve your desired results. https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/time/LocalDate.html#format(java.time.format.DateTimeFormatter)

It seems like your TaskList is doing the job of Ui returning String objects to be printed/shown to user.

A switch case would be apt here.

I think it will be good to give the Storage object a proper name.

Else, you should probably use a switch case instead of many else if blocks. Remember to indent correctly if you do go with switch case.

Nice additional messages

Maybe you could retrieve "data/savedList.txt" as a method argument instead of hard coding it right here. So, maybe use the method header instead:

public void exportData(String path) throws IOException

You can probably convert type into an enumeration if you want to.

Perhaps it would be good to see header comments here.

Perhaps this could be updated to fileName?

Maybe 'isActive' would be a better representation of the boolean nature of this variable?

Perhaps Ui or UserInterface may prevent abbreviations from being misunderstood or taken out of context.

I think adding header comments in the different files in your project would be helpful to aid readers to understand their main purpose and functionality 😄

Perhaps this could also be updated to reflect the boolean nature?

Perhaps it would be nice to have some JavaDocs comments reflecting the nature of each method.

I like the clean nature of the messages returned! Perhaps the Ui could do a bit less stuff (e.g. find) and abstract out further? (SLAP)

Perhaps adding author and version tags to the javadocs may be good as well?

I like the clean regex used to filter commands!

I also like that you also declared constants to prevent numbers from appearing in the code with no context later!

Perhaps it could be standardized to use the "tasks" convention here?

Another instance for standardization of taskList

I like the verbosity here, although I wonder if it could be made more user-friendly?

Even though 'rmb' is just a temporary short-lived variable, would a more readable variable name make things easier for other collaborators and when looking back in future?

Could a switch case be a neater alternative as compared to a long list of if-else?

I noticed a few repetitions throughout the various classes where you had to use multiple if else statements to display the event type. Have you thought about handling the display of an event type in the respective toString() methods of the Task? Would that be a more efficient way?

Noticed that a few of your boolean variables were not named to sound like boolean variables. E.g. the 'first' and 'done' booleans. In this case, would it be more intuitive to name this boolean variable as isDone instead?

Is there a reason why you are hardcoding the tabs? Would '\t' may be a neater way of displaying your tabs?

I agree with @wangtao0717 on the Duke.java being a little too long, maybe you could look into that.

Since you do not plan to use dueDate or taskDescription, could you directly place parser.getDueDate() and parser.getDescription() in the constructor instead?

I like how you let the switch cases fall through for commands requiring the same output. Something most people forget about.

Would it be better to leave an empty line between the description and parameter section of the Javadocs (increases readability too)? The description should also begin with 'Initialises'. Just to be in line with the module's conventions.

Perhaps these additional newlines should be removed to stay in in line with the coding standards. I noticed the same for a few methods as well.

Would it be better to use a more intuitive variable name e.g. tasks?

Perhaps for greater readability for future collaborations or debugging, the dataSplit could be assigned to multiple variables with a more intuitive name? E.g. taskDescription, taskDeadline

Would it be more consistent to rename your Duke class to Duck just to be more consistent with your project name?

I agree with @kouyk that the HORIZON variable could be renamed to something more appropriate and intuitive. You may also want to consider setting it to final since there is no intention of modifying the static string anymore.

If i have a command

'event todo a task /by tmr'

this string contains 'todo' but it is actually event...

i think testing prefix maybe safer?

i think over here, by coding standard, the + should be the start of the preceding line hh,

same mistake made bro.

could you please double check if static import needs to be imported before non-static, i think so but i am not 100% sure haha

public class and methods undocumented bro, small issue, maybe u will do it later haha.

i think public constructor needs a documentation also

maybe import of java.io should be in front of java.nio/util coz of lexicographic order

Doesn't public class need header documentation also?

same as above for documentation in my opinion

this method needs documentation too haha

this deep nested loop can be simplified perhaps? but i actually have something similar myself haha, maybe i should look into it too...

do you think this name should be doesFileExisits maybe?

I remember the start of method documentation should be third-person singular? like Saves Task list as ....

this method needs comment also i think

All these public methods not documented sia... are u planning to document them later maybe?

Should the this be removed?

Perhaps there should be a space between for and ( ?

Should the + operator be on the next line?

Should the + operator be on the next line?

I have noted the issue in several other places as well...

Perhaps there should be a space between Exception and { ?

Should there be curly braces for the if clause?

Should there be a space between for and ( , if and ( ?

I have noticed this issue in several other places...

Should there be a space between operators and operands?

Perhaps the indentation for switch and case should be the same?

You may find this guide helpful 😄

Should there be an empty line separating the description and parameters?

Should the redundant this be removed?

Should timeslot be in camelCase?

Should there be a space between switch and (?

Perhaps entry[2] should be declared as a variable with a descriptive name for better code readability?

should for statements have the form: for (initialization; condition; update) { ?

should there be one space after for?

I noticed the same issue in several other places too.

I like how you order the import statements

Will it be better if your addItem method is named as addTask instead? I think it may better suit the context..

should there be a space after for ?

and should operators be surrounded by a space character?

Perhaps typeOfEvent should be changed to a more intuitive variable name? Since a todo is not an event.

Please use better variable names. Perhaps a more specific name relating to Todo?

I like how similar methods in Ui class have a similar naming format.

I like how your ordering of import statements stay consistent throughout the work.

according to your ordering of import statements, should there be an empty line between the first import statement and the next 😄?

should boolean variables be named to sound like booleans?

I like how you name this method readTasks instead of readTask, very intuitive!

I like how "tasks" in this class is in plural form.

Would it be better to name this method "makeSenseOfUserCommand" instead? Other than that, I like how the method name clearly explains what the method does!

Perhaps naming this String "output" could be more meaningful?

I think naming this variable "taskIndex" or something like that could help improve readability? Especially since you will keep using it repeatedly in the else block below.

Maybe it would be clearer to mention in the variable name what parts these are?

For example:

"inputParts" instead of just "parts",

"inputFirstPart" instead of "part1",

"inputSecondPart" instead of "part2"

Perhaps replacing "xx" with "localDate" and then replacing "f" with "formattedDateString" could improve readability?

I noticed that you are using 2 counters so you named them counterOne and counterTwo. I like the fact that you try to differentiate them, but maybe it would be more intuitive to name them something else?

Would it be better to name this "loadFile" instead?

Similarly, would it be better to name this "saveFile" instead?

This might be minor, but do you think naming the variable as "file" instead of "f" would improve readability?

Similar to the file variable naming, perhaps naming the Task "task" instead of "t" could make things clearer as "t" could also stand for "todo"?

I like how you labeled the Task here as "task" instead of just "t", it becomes a lot easier to follow!

I noticed that earlier you used the variable name "description" for the String array after splitting, while here you also use the name "description" for a String type, I think it might be a bit confusing? Maybe labelling them as "descriptionArray" and "descriptionString" could make things clearer?

I think constant names are supposed to be all uppercase with an underscore to separate words? Unless I misread and this actually isn't a constant oops

I like how clear all the variable names are!

this class is rather long, perhaps you could consider abstracting the methods out?

delString is not a very intuitive variable name, maybe you could make it more specific?

i think the term you meant to use might be "overloaded" instead

I think that constants in enum should be in all uppercase, eg "LOW", "AVERAGE" and "HIGH"

Did you mean "Overloaded" here as well? I've noticed it in a few other places. Otherwise, as the other reviewer mentioned, overridden methods should tagged with "@Override"

perhaps some empty lines could be removed to make the code look more concise. I've noticed this in some other places as well.

the switch statement as provided in the coding standard has no curly braces, and an explicit "//Fallthrough" comment should be included whenever there is a case statement without a break statement. it's a minor matter but perhaps you could add this in 😃

i agree with the other reviewer, this is much more efficient than the method i used! (explicitly considering the cases of 0 or 1 task)

perhaps this comment is incomplete? im not too sure what you mean by "Stage to show"

the java coding standard suggests that "The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement.", perhaps you could consider adding it in!

i agree with the other reviewer on using "commandDetail" instead

this is a smart way of identifying done and delete tasks ! 😃

very clear and efficient Exception class 😃

perhaps this line could be broken up for better readability

My understanding of the CS2103 Java Coding Standard is that Javadocs should end with */ rather than **/.

2 points:

1. There should be an "Empty line between description and parameter section" - CS2103 Java Coding Standard.

2. There should be "Punctuation behind each parameter description". I.e. Lines 12 and 13 of Deadline.java should end with a period, as shown below.

Consider the following Javadoc:

/**

* Constructor of a Deadline task.

* 

* @param desc Description of the deadline task.

* @param deadline Deadline of the deadline task.

*/

The line break here uses 12 spaces instead of the required 8.

The line break here uses 12 spaces instead of the required 8.

The CS2103 Java Coding Standard gives the following examples of acceptable ternary statements:

alpha = (aLongBooleanExpression) ? beta : gamma;

alpha = (aLongBooleanExpression)

        ? beta

        : gamma;

Consider enclosing the boolean term within parentheses, as in the following:

String keyword = (firstSpace == -1) ? fullCommand : fullCommand.substring(0, firstSpace).toLowerCase();

Could consider ending the last case statement with a 'break'.

From the CS2103 Java Coding Standard:

The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement.

Rationale: Leaving out the break is a common error, and it must be made clear that it is intentional when it is not there.

From the CS2103 Java Coding Standard:

Here are two acceptable ways to format ternary expressions:

alpha = (aLongBooleanExpression) ? beta : gamma;

alpha = (aLongBooleanExpression)

        ? beta

        : gamma;

Could consider placing the parentheses around the isDone clause rather than the whole return term.

Consecutive line breaks should have an additional 8 white spaces.

Example from CS2103 Java Coding Standard:

method(param1,

        object.method()

                .method2(),

        param3);

Note the 8 white spaces preceding object.method() and 16 white spaces preceding .method2(),

Could consider ending the Javadoc summary statement with a period as was done in other Javadoc summary statements in this PR, for consistency.

Each consecutive line break for the same line of code should be indented by an additional 8 whitespaces.

Consider replacing lines 49 to 51 with:

        dialogContainer.getChildren().addAll(

                DialogBox.getUserDialog(input, userImage),

                        DialogBox.getDukeDialog(response, dukeImage)

From the CS2103 Java coding standard, Javadoc should have "Punctuation behind each parameter description -No blank line between the documentation block and the method/class".

I'm unsure if this applies to the first sentence and return type but in other classes of this PR (e.g. Parser.java) the first sentence and return type of the Javadoc both end with a period.

Consistency of punctuation in Javadocs across the various classes might be a way to improve this PR.

Admittedly this is a nitpick.

Though you probably already satisfy the "Javadoc more than 50% of public classes and methods" requirements, a useful tip is that by adding Javadocs to a class with methods to be overriden such as the abstract Command class, these Javadocs will appear in Quick Tip windows when users however over overriden implementations.

E.g. Writing a javadoc for getResponse in the Command class will show users the Javadoc when they activate Quick Tip (Ctrl + Q in IntelliJ) for an overriden method in the DeleteTaskCommand class even though there isnt a Javadoc written in the DeleteTaskCommand class.

I agree with @jrvslam in that there should be no indentation before the case statements.

If IntelliJ is your IDE, you can follow the instructions at the following link for how to get IntelliJ to follow this coding convention for you.

https://stackoverflow.com/questions/40888798/config-intellij-switch-case-style#:~:text=Go to Settings > Editor > Code,Indent 'case' branches".

Should there be a documentation here to tell us about the functionalities of getSnomDialog?

Should there be a documentation here to tell us what does this class represent?

Should there be documentations before every class?

Concise and clear documentation, really helped me to understand the functionality of this method. Good job:D

It would be good to have documentation for every public class

Great namings for all these methods, really shows what does each method do

Should the switch and case have the same indentations?

Great structure of conditional statements and try catch blocks, very clear and concise for readers to understand!

Should we use curly braces to enclose the break to improve readability?

A pretty long stretch of code, would it be possible to abstract further?

Very neat and concise layout, good job!

method calls lied out according to similar functionalities, good job!

Good use of abstraction!

Perhaps the deadline formatting can be abstracted out to a getter, which would make this bit look cleaner. This issue exists in some other parts of the code base too.

It might be helpful to remove the else clause so that the core logic is not so deeply nested, and the happy path is more prominent

I believe based on the style guides, the case statements should not be indented. Perhaps double-check this?

Is there a particular reason you are using the enhanced for loop here? I see that you have an index variable, maybe a normal for loop would then suffice, as the iterator is contained within the loop's scope

Perhaps seq can be a more intuitive name, or a comment explaining what it does would be helpful

It might be easier to maintain if the case constants are extracted out somewhere else, either as an enum or some constant variables

This might be able to be abstracted out using the String.join function.

This kind of abstraction may be applicable in a few other parts of the code base too.

I see that itemList and fileContents are frequently modified in the same way (addition/update/removal) together, maybe abstracting out to a function would be cleaner and easier to maintain

Perhaps it might be more future-proof to use itemList.count() instead of manually maintaining a count value. Is there a reason you did it this way?

Perhaps all the file-based IO can be abstracted away to another class for easier setup/teardown and CRUD operations. Is there a reason to keep it purely within the main class?

It might assist readability to break the .append into multiple lines since they are new lines, and it makes it clear what is being appended to the StringBuilder

It might assist readability to break the .append into multiple lines since they are new lines, and it makes it clear what is being appended to the StringBuilder.

This specific style issue exists in a few other places too.

Perhaps the same breaking into multiple lines argument goes here too to improve readability, and ease of maintenance should the order be changed or new commands be added.

Instead of getIsBye(), perhaps isBye() would be more intuitive and compact.

I think you can consider adding the @Override decorator here

I like how clean your driver class is for the entire program!

I like the use of regex to parse the input command!

Perhaps a switch statement would be suitable here! 😃

I like that you've given your bot a name!

Should non-constants be uppercase?

I think that instead of getIsDone(), isDone() would be more succinct and compact.

I think that some white spaces can be added here

    Task(String name, Boolean isDone) {

I think that white spaces can be added here for readability

        assertEquals(task.toString(), task2.toString(), "Wrong format");

As a suggestion, perhaps define these DataTimeFormatter patterns as static and final constants as they seem to be used quite a number of time throughout your code.

I think that it would be a good idea to remove dead code here to better readability.

I think that a switch block here would help to improve readability.

Perhaps this could be an enumeration instead?

Many of your cases start with command.toLowerCase(), should this could be abstracted out as a variable instead?

This method seems a little long. Would it be better to abstract out some of the logic into other methods instead?

Perhaps this response could be added to your PrintText class?

Perhaps this should be 4 spaces rather than a tab? I noticed you used tabs for indentation in some of your other classes too.

Perhaps there should be an empty line between description and parameter sections, as well as punctuation after each parameter description? I noticed this in your other javadoc comments too.

Perhaps there should be a javadoc comment here?

Perhaps while should be followed by a whitespace character?

Perhaps these lines should all be indented at the same level?

Perhaps there should be a javadoc since this is a public class? I noticed the same issue in some of your other classes too.

Perhaps list should be in plural form?

Perhaps the first word of the javadoc should be Returns?

Perhaps the name of the method should be a verb instead? I noticed some of your other methods have the same issue.

I think it was a good idea to separate the project into task and body packages, but according to the module's coding standard, names representing packages should all be in lower case

You may consider writing your variable names in camelCase? So perhaps userCommand instead of usercommand

According to the module's coding standard, there should not be an indentation for case clauses

Since logo string is a constant, you can consider making it uppercase as per the module's coding naming standards

Hi, according to the module's coding standard, single statement conditionals should still be wrapped by curly brackets

I was wondering if it is bad practice to leave the catch block empty? Maybe you can consider adding a statement to print the error message.

Is it possible to make this protected? The coding standard says class variables should not be public

Since this string is a constant, maybe you can make it static and uppercase?

According to the module coding standard, there should not be indentation for case clauses

I noticed that you often used the work "process..." when naming your functions. I think process is kinda vague and may confuse the reader. Maybe you can consider a change like convertDescriptionStringTo... instead of processDescription

According to the module's coding standards, there should not be an indentation for case clauses

Hi, is the exitFlag supposed to be used to check for when to exit the while loop? I was a little confused as the while loop used while(sc.hasNextLine()) and I couldn't find where the exitFlag was used

This method is a little long, so perhaps reducing some of the empty lines might improve readability.

Missing access modifier for this variable.

missing access modifier and Javadoc comment for this constructor.

missing access modifier and Javadoc comment for this constructor.

Missing Javadoc comment

Missing Javadoc comment.

Missing access modifier.

Similar to Deadline class, this class needs access modifier for the constructors and Javadoc comment.

This line should be put before the "import duke.Task" line.

Missing Javadoc comment for this part.

The plus sign should be put in the next line.

Missing Javadoc comment for this class.

There should be a space between Exception and {

Missing Javadoc comment for this class

instead of using b, you could just use isDone.

You could have a better convention such as dateTime. "by" alone doesn't have any meaning.

Should be lists instead of list. Also, the naming convention could have been better, perhaps using tasks is better? After all, your code is being read by many people with different backgrounds. So it would be better if your naming convention is clearer.

Your deadline by naming convention should have followed this format as well.

You can change the if-else to switch case for better readability.

Could have just do for (int i = 1; i >= list.size(); i++)

You should catch the DukeException, if not your program would stop running due to the exception.

It seems like getStatusIcon() is used in this task class only, hence you can change public to private.

Indentation error

Maybe could have abstract each if-else statement into functions to increase readability.

It seems like your parsing for each command is pretty long. Could have broken down into functions to increase readability.

Instead of starr, it should be strArr. It's clearer to the readers this way.

Preferably to have methods start with a verb.

Would it be more readable by adding blank lines around separate logic blocks?

Perhaps a name like getTime can improve the clarity?

Perhaps using Enums here would be more suitable?

Would it be better to remove such comments or elaborate on them so that they are more understandable?

The function name is rather self-explanatory. Could the javadoc be removed without losing code quality?

Would it be clearer to give the function a name like excecuteDoneCommand? This applies to other command related functions as well.

Perhaps deleting old code can enhance readability?

Perhaps giving a more descriptive name will be better as this variable is used in later codes? This applies to other areas in the code too.

Perhaps a better name would be isTaskDone?

Might be better to add a line between logic blocks to enhance readability?

Will it be better to name this like a question (ie. hasKeyword)?

Would it be better to add separate the logic block with one blank line? This suggestion also applies to other parts of this file with if-blocks.

I like how neat the import statements are 👍

LGTM. Perhaps add some javadoc for your public class/methods? (applicable to other classes as well) 😄

Maybe can allign the '//' to your code? Like:

    // Initialize Task container

    ArrayList<Task> tasks = new ArrayList<>();

I would say adding an extra while space here?

    while (!input.equals("bye")) { 

    ...

I guess can try to configure your IDEA to apply the same indentation for switch as in the coding standard?

    switch (command) {

    case "bye":

        // ...

    case "list":

        // ...

Wanna replace i+1 with i + 1? trivial issue though. 😃

I guess a white space can be placed here after catch, as per coding standards. 🤔

Same white space issue as above 😃

Like this easy-to-change-name idea! Didn't think of it 😄

Handle -> Handles 🤔

I guess can try working on abstracting some methods out? (SLAP) 🤔

Same for below.

Love this! Default branch used. 😄

The () becomes a bit deep here? I am not sure whether this is related to code quality but I guess can extract out a few lines out of this single line to make it more readable? 🤔

Do you think the deleteTask and completeTask share some similarities here? What about abstracting something out? 🤔

Perhaps you could use isDone instead of b, so that it is more descriptive?

For greater readability, perhaps you could have a whitespace after the comma?

Should there be an empty line before this constructor?

I notice that there are times you leave an empty line before constructors, and there are times you leave out the empty line.

Same sentiment as the above!

Perhaps you could try having this over two lines?

Quoting the java coding standards:

Try to keep line length shorter than 110 characters (soft limit). But it is OK to exceed the limit slightly (hard limit: 120 chars).

I think the line currently is 110+ characters, but it may improve readability slightly if placed over two lines.

Might be good to add an empty line between the class constructor and the method, so that there is better readability?

I see an empty line here, but not in your constructors for your other classes. Perhaps you could remove this empty line to make it consistent with the rest?

Not sure if you should still keep this commented-out code here since it seems to have already been replaced with an updated version below?

I might have missed it, but there doesn't seem to be use of the constant INDENT in subsequent parts of your code. Do correct me if I am mistaken.

I like that you made an effort to distinguish between singular and plural tasks! 😃

Perhaps you could try exploring the use of "\t" to create tabs, instead of coding the spaces out.

I agree with the above. While it is possible to understand currently, the use of String.format() may improve understandability further.

Should the class be added or grouped inside a package? This applies to other classes as well.

Will it be better to abstract and break this method into many smaller units? It will help in the future also.

Will it be better to add javadoc to all public classes and methods? The same applies to all other public classes and methods.

Can I check if this import is being used? Because Array should be present by default in Java.

so sorry I thought this was my iP, please ignore my replies if any:)

Will it be better to remove codes commented out when pushed to master? If necessary, can store them in another branch:)

Should it be "Intialises" rather than "Initialise"? A full stop "." is missing at the end of sentence.

Should the name of the method be "getTask" or "getTaskInfo"? This is because this method is not "printing" the task information and it is more to return the task information before it gets printed.

Very minor point here. Will it be better to change "keyWord" to "keyword" as it is a single word?

Should "set" be changed to "isDone" since it is a boolean?

Should "done" be changed to "isDone" since it is a boolean?

Should the pakage name be more specific? For example, those about UI can be undert the package ui and those about storage can be about storage.

could the variable "by" have a better and more meaningful name?

Overall code is very easy to read. It would be made easier to read if some method and variable names were longer and their short forms were not used, eg "numDone" and "isEmptyDesc"

would a more meaningful name be "venue" rather than "at"?

I like that the method names in this class are all starting with verbs 😃

Would "tasks" be more efficient in telling the reader what the content of list while reading the code?

im not sure, but the eg given in Java coding standards seems to do something like (isDone) ? "X" : " ";

could there be a more meaningful variable name as compared to "by"?

maybe str could be changed to action to prevent the use of shortforms, and for increased readability

Overall very easy-to-read codes, however would explicitly writing out the variable names like currTask and printErr improve the readability?

a bit of an "arrowhead" code here, maybe can create another method for code in the while loop

would it be better if each subclass had a create() method?

maybe the convention of naming test methods could be checked up

I like that you kept your main class clean and simple!

This try-catch section is quite deeply nested. Perhaps it could be avoided by catching the DateTimeParseException together with the DukeException in the catch clause below.

Perhaps this boolean variable could be better named to sound like a boolean. E.g. shouldExit

I think a cleaner alternative to having such a long if expression here would be to use a switch statement instead. E.g.

switch (command) {

case "todo":

    // Fallthrough

case "deadline":

    // Fallthrough

case "event":

    // Fallthrough

    cmd = new AddTaskCommand(command, input, tasks);

    break;

case "list":

...

Might be good to avoid using magic numbers here. Instead of 9 perhaps you could have a constant string for "deadline " and run the length function on the string?

Perhaps a cleaner alternative would be to return expression directly?

return input.equals("list") || input.equals("list ");

👀

nit: there seems to be a minor typo here with two semicolons

Perhaps this method name could be clearer if a verb was used. E.g. markTaskAsDone

I'm not entirely sure if this counts as a coding standard violation, but perhaps you could remove the individual braces from each case statement.

I think the ending brace should be with the last line of the statement, instead of being on its own new line. I noticed the same issue in several other places too.

Perhaps a better name for the setter method of the isDone boolean variable could be setDone

Could consider calling lines() in the greet() method (and other places where you call it) instead of in the main, where lines are very precious. ie. in the greet method:

'greet() {

ui.lines();

...

ui.lines()

}'

love the concept of Chat the cat

incredible stuff

i think its more standard to use the standard for loop in this scenario, even though they do the same thing... so that the code can be less surprising and more standard

the structure here is abit hard to follow... since "Please input with this ... " occurs quite alot in the exceptions, maybe can refactor that phrase out of all the exceptions which point to formatting error?

seriously very nice docs

hmm isnt the standard to capitalize the first words, eg. CreateEvent_NoSpace..._ChatException

^*!_ &^%%**$$$$

Would it be better to just not set it instead of setting it to null? Just wondering... same for the tempDate below

Maybe it would be good to include some short comments on what these variables are doing in the code haha... took me a while to understand the method 😛

Just smt to consider: showError may confuse ppl a little since its usually associated with printing to stderr, but I guess its not a big deal at the same time 🤷

Not sure if you meant to not capitalize the "the over..."

and also perhaps across classes, might want to standardize the wording for overloaded constructors (eg. with Event class)

Should there be a new line here?

Some of the new line spacing is a bit inconsistent. Perhaps you can standardize whether you're putting a new line spacing after every curly brace

I like how you split up the code into separate lines! Much more readable

Perhaps this can be named Deadline instead of Deadlines, since it is a single Task

Shouldn't this be clearAllTasks instead?

Perhaps can rename this to makeBiggerBox

Should name this as an action, like convert

I like how everything is packaged! Very neat and clear 😃

I like how you split this up into two lines!

Perhaps this is a little vague. Could be more descriptive!

Is this findString or should it be findTask?

I think it could be good practice to add a line spacing between import statements from different packages!

Should you use Egyptian style brackets instead, in line of the module's coding standards?

Should the case clauses have the same indentation as switch?

Shouldn't the indentation be 4 spaces instead of a single tab?

Shouldn't the imported classes be listed explicitly, instead of using a wildcard?

Should the method's name be something clearer? For example getCommandNameFromToken

Should you remove the space after the method name?

Additionally, should you use a more meaningful method name? For example createCommandFromString

Should you remove the space after method's name here as well?

Should you use Camel case to name this class name, for consistency?

Should you remove the space after method's name?

Should you have an empty line between the description and params?

Should you name the class as a noun instead? e.g. CommandParser

Should you name this as saveTask instead?

Perhaps it would be better to name this Todo instance as todo instead of t, to improve the readability of the code.

Similar to the comment on naming the ToDo instance, maybe it would be better to name this as deadline instead of d. I noticed the same issue in a few other places too.

Maybe you can leave an empty line between the description and parameter section to comply with the coding standard.

Maybe you can leave an empty line between the description and parameter section to comply with the coding standard. Same for the remaining Javadoc comments!

Even though the line length is lesser than 120 chars, but perhaps it is better to keep it short and neat. Maybe you can break at 'throws InvalidIndexInputException....' to improve the readability of the code. Same for other similar methods!

There are many parameters to this DeadlineCommand object. Perhaps by breaking after a comma would improve the readability of the code?

Should these empty lines be removed?

Should these empty lines be removed? I noticed the same issue in a few other places too.

Perhaps you can use Egyptian style brackets here!

Would this method be clearer to the readers if the method name is named to sound like a boolean? Perhaps a suggestion like 'isExit' would be more suitable?

Perhaps you can add a full stop behind each parameter description to comply with the Javadoc comments form! Same for the other Javadoc comments.

Would it be better if the imported classes are listed explicitly?

The code follows the Java Coding Standard and with good descriptive header comments. Overall, it gives the code really good readability to allow anyone to understand the implementation of the Duke class with ease.

A trivial violation of the Java Coding Standards as name is not in camel case. A quick edit should solve it!

Code follows the Java Coding Standards and is really readable.

Good coding style which follows the Java Coding Standards. Could consider adding header comments for the function methods to improve ease of understanding and readability of the code.

Follows Java Coding Standards which is good. A suggestion would be to add header comments to methods being override to improve readability and ease of understanding especially when tenary operator is used which could make it slightly less readable.

Similar to Deadline Class. The code follows Java Coding Standards and a suggestion would be to add header comments to methods being override.

Code is really readable and follows Java Coding Standards. Could consider adding header comments to the getResponse() method to improve ease of understanding.

I agree with @yeoutzer and like how the function names are descriptive and there are header comments to the functions which improves ease of understanding.

I like the consistency in the quality of the coding style which adheres to coding standards as well. The header comments are sufficiently descriptive which improves readability too.

I agree with @yeoutzer. The function names use verbs which not only follows the quality guideline but makes it easy to understand purpose of each method.

I like that you indicate a comment at the side to indicate what unicode symbols will be printed out.

Would it be better if the javadoc comments started with a capital letter and ended with a punctuation? It would also be good if there is a description after "@throws DukeException", perhaps "@throws DukeException If input is incorrect."

I think it would also be good to remove the empty line between the javadoc comments and the start of the method.

Should this DukeException message be written specifically for creating Deadline tasks? Perhaps you could use the string "The description and due date of a Deadline cannot be empty."

Should there be an empty line between the method description and the parameters? Similar to before, it would be good if the javadoc comments start with a capital letter and end with a punctuation.

Would it be better to use a more comprehensive name instead of "tl"?

I noticed this same coding violation across a few files. Perhaps it would be better to change these javadoc comments to fit the coding standards.

Would it be better to create a summary of the method to use as the first sentence? Perhaps the first sentence can be "Initialises Duke with a storage file." followed by the next sentence explaining more about the specifics of the method.

Should these descriptions start with a capital letter and end with a punctuation? I noticed this in a few other javadoc comments too.

I have seen a few of such javadoc comments and descriptions without punctuations. Should these comments end with a punctuation?

Should the first sentence be a short summary of the method? Perhaps "Creates dialog boxes to display.", followed by the specifics of the method as Javadoc places the first sentence in the method summary table.

Similar to before, would it be better to use a short summary for the first line before explaining the method in detail?

Perhaps add a JavaDoc for this method? Similarly for other public methods.

Perhaps the () are redundant and could be removed?

Perhaps the this can be removed since file is not being shadowed. Similarly for other places.

Maybe remove the extra {} for the case block?

Perhaps Adds would fit better here? Similarly for other places

Perhaps you could abstract the numbers out to a static final variable to avoid magic numbers?

Good catch for the bye command to show the message before the program closes

Perhaps you should abstract the different commands to their respective functions to avoid the long parse method.

You might want to add a blank line in between the description and @return. Reference for code style here https://se-education.org/guides/conventions/java/index.html#comments

Similarly for all other javadocs

Maybe it would be better to call it ìsDone'for boolean variables?

Perhaps you should rename the method as the name does not reflect its behaviour?

Maybe list out all imports explicitly instead of *

Maybe add some javadocs so others would know what youre trying to do in this class

Nice use of plural since it is an arraylist of tasks

Maybe name the boolean as a question isTaskDone

I feel dont need to so explicitly show what will be returned since the toString method is quite clear

nice use of switch statement to make the code clean 😃

nice explaining why you return an empty string

Maybe add some javadocs here

Maybe change it to isTaskDone since it is boolean naming

Maybe just updateItem since it is known ArrayLists are mutable

Maybe add javadocs explaining what this class is

Perhaps it is 'Creates' instead of 'Create'?

... I noticed the same minor grammatical issue in other places too

Should this new line be removed?

Should these lines between class declaration and constructor be removed?

Perhaps Javadoc comments could be added for the constructor?

I like how you further extract Command into child classes 😃

Perhaps location is supposed to be timing?

Should it be in plural form since it is an array?

Is it showGoodbye or showGoodBye?

Perhaps add javadoc comments for public methods?

Perhaps timing should be date so that it is more consistent with the deadline task?

Perhaps a more intuitive name for this method that clarifies what data and input are being referred to?

Perhaps a more intuitive method name that describes what is being operated should be used since list is generic and could refer to other listings like help options. I noticed the same in several other files as well so it might be good to re-look at class methods that might not be clear in conveying what they do 😃

Since this isn't a constructor, setter or getter, perhaps a header comment should be included here?

Perhaps the import statements for java packages should come before those of third party packages? I noticed this in the imports of other files as well so it might be good to take a look 😃

Would a more intuitive variable name like taskList or tasks convey more clarity?

Perhaps a verb in front of this method name such as showExitMessage() would make its function clearer. It might be good to consider the same approach for naming other methods as well to make them all easier to read 😃

It might be better to import the classes individually instead of doing a wildcard import 😃

Since this isn't a constructor, setter or getter, perhaps there should be a header comment for this method as well?

It might be best to capitalise the names of constants even if they are private attributes. I noticed this in several other files too so it might be good to do a quick check on this 😃 For your convenience, the bottom of this quick guide shares how the checkstyle.xml can be updated to catch these violations 😃

Should the import statements for java packages come before the import statements for third party packages?

Unless the application is behaving incorrectly with the final keyword, I am inclined to think that capitalizing the instance attributes would be the better option. The final keyword reliably ensures that the state of the attribute is never modified and this seems especially important for the taskList, which should not have a new list object unintentionally overwriting it. I have to admit I am not too experienced with java and have only read up on the use of final recently so I am open to and would appreciate any advice on this as well 😄

Would it be possible to combine these method calls from ui into one method?

Perhaps there may be a better way instead of nesting if else statements, an alternative would be using switch cases

JavaDoc can be slightly more descriptive about the workings of this method and what it does

This method could possibly be broken up as well and could better adhere to Java Coding Standards

Naming conventions can be better adhered here

Java documentation clearly defines the method and is very readable! Good job!

A clear and concise name for the method! Also a good JavaDoc header that describes this method!

According to Java coding standard, is it better to make it private here?

Adherence of the java Coding standards! Good Job!

Very good separation of imports here

Would it be good to have a line break to improve readability?

Task t = tasks.remove(this.taskIndex)

I think can make this simpler by combining both statements.

Can have a space after between Command and {

Can explain why the method always returns true. eg So that duke can continue accepting commands. (I understand this after reading the ExitCommand)

I think can give a variable to t.toString() to explain whether this is the task description or the string representation of the task.

This field should be right after header?

Add a header comment to describe the use of this task.

I think can declare Task as an abstract class since there is no Task instance.

Can give a more descriptive name, ie what does kw stands for?

Is this method a bit too lengthy, can consider extract out the common part of the code.

This indentation is off.

Add a default case as specific in textbook.

I think the method body can put in a separate line so that it is more consistent with other method style.

        while (scanner.hasNext()) {

I think you need whitespace around this

Same as what now? Please clarify in place

    } catch (DukeEmptyCommandException e) {

            System.out.println(e.toString());

        } catch (DukeWrongCommandException w) {

            System.out.println(w.toString());

I think adding white space is good practice here?

    public void sayGreeting() {

I think a verb here is good? Also white space

                for (int i = 0; i < len; i++) {

                    int index = i + 1;

                    Task t = this.list.get(i);

                    if (t instanceof Todo) {

                        Todo todo = (Todo) t;

                        System.out.println(index + "." + todo.toString());

                    } else if ( t instanceof Event) {

                        Event event = (Event) t;

                        System.out.println(index + "." + event.toString());

                    } else if ( t instanceof Deadline) {

I think you need some white space here

I'm not sure whether this is an acceptable break from indentation for wrapped lines having 8 spaces? Also, I would suggest you align the pluses with the equal sign, not with the first double quote symbol

I think you might want to separate java.util from java.io? MIght be pedantic.

I think you could make the method name less misleading? Right now my first impression is that it performs the first step in all parsing. Perhaps you could rename it to "parseLoadedData"

While this is fine, I think you could differentiate this from "parseToStart" as "parseUserInput"?

I think you need to make this a verb, perhaps "markTaskAsDone"?

Very descriptive name! I like 👍

In method header comments, the first sentence should start in the form "Deletes..."

returns instead of return

should the boolean variable be named isTaskDone instead

I agree with Anli and Eriksen regarding this

"Sets" instead of set?

Are you supposed to add content to this test class file, because this class does not serve a purpose now as it is empty

should this be "A GUI for Lihua..." to be consistent with the name you decided to name your bot

I agree with this. variable names should be in camelCase

Should name boolean variables to sound like booleans i.e hasBy instead of foundBy?

@param is missing

Should the first sentence of method be "Executes" instead of Execute?

Perhaps you can consider using else if and else statements here instead of a nested if-else to improve code readability? I noticed the same in other places too.

I like how you used a relative path here 👍

Should this function be renamed to 'replaceResponse' or 'newResponse' to make the intent of the function clearer? I wasn't sure what 'replace; did when I encountered the function in the Parser.java file.

Should the logic used extract out the description be simplified to make it more understandable? Perhaps instead of splitting on spaces and combing all the words after todo, you can simply remove the word 'todo' from the String? This will make require only a single line of code and is much more compact.

Something like:

String description  = line.replace("todo", "").strip()  //.strip() is to remove trailing spaces

The same can be applied in other parts of your code for descriptions of deadlines and events.

I like the intuitive naming of all the functions in this class.

Perhaps you can consider abstracting away the formatting/extraction of dates/times by creating a separate class for functions related to these? There are a lot of variables declared here and abstracting these details will make the code easier to understand.

Should you leverage the LocalDate or some other library to convert the date and time format? I feel that doing so will make the code less cluttered and save you the trouble of writing everything from scratch. This can also be applied to all the classes which inherit from Task.

Maybe consider something along the lines of this tutorial?

Perhaps changing the default JavaDoc comments to something better descriptive of getResponse would be clearer?

I like how the static variables are used to keep track of the command types to improve the code readability. Perhaps you could consider wrapping all these static variables in a static class to make it a bit more organised.

Something like:

private static class CommandTypes

{

    public static final String TODO = "todo";

    public static final String DELETE = "delete";

}

Perhaps all the nested if-else statements are a bit difficult to read and follow? Maybe you can try using switch statements?

I like the intuitive naming of all the functions which makes it easy to understand

Should variables that describe a collection values be named in the plural sense?

Should there be newlines separating these methods for better readability?

Should this method be named with a more suiting name? Perhaps a name that explains why this method has to return an array of Strings?

Should this constant be named with all uppercase characters?

I like how the string is split between multiple lines 😄

Perhaps include some whitespaces in the for statement?

Should this line of comment start with a third person present tense?

You might have forgotten to put a whitespace around the if statement.

Should variables which represent a collection of values be named in the plural sense?

I like the way this long string is split between multiple lines 😄

Should the if statement be wrapped in curly brackets even when they are single statement conditionals?

Header comment is missing for this public method.

Header comment is missing for this public method.

Header comment is missing for this public method.

Header comment is missing for this public method.

Header comment is missing for this public method.

Comment lacks @param and @return tags

@return tag is missing from this comment.

Missing @return tag in comment.

Can consider changing parameter name to something more meaningful, e.g. "description" 😄

Can consider changing parameter name to something more meaningful, e.g. "keyword" 😄

Missing @return tag in comment.

Do you think it will be better if you insert a whitespace between ')' and '{'?

Do you think it will be more organised if the case blocks are indented at the same level as the switch block?

Do you think it would be better to import only the necessary libraries?

Remember to include newlines at the end of each file!

Did you accidentally leave out the description for the parameter?

Just my opinion: any reason why you chose to use a normal class intead of an abstract class? I notice that you never instantiate a Task object in your main code anyway.

Do you think this comment is misleading? Since Task cannot be created but rather Task's children classes.

Is this lst variable meant to be accessed outside this class? If not, perhaps a getter method would be more appropriate rather than giving direct access to the other classes, which would result in undesirable side effects.

Any reason for the choice of name for this method? The name suggests that it will return Tasks, but instead if gets the lst and returns it. Would it be more appropriate to call it getXXX instead? Just some thoughts!

Would it be better to split the logic into different methods for better organisation?

Do you think it would be clearer to explain that this method is only called when read from storage?

The method here is a bit too long. Would it be possible to make this method slightly shorter by adding helper methods?

This method seems too long as well. Maybe you could add the switch case part to a separate part?

As others mentioned, perhaps you can rename the variable old to something that could give an indication of what it is used for.

Perhaps you can rename the localDate "by" with something that indicates that you are dealing with a local date, since when it is used in a method, we may not immediately understand what it is used for.

Perhaps you could have initialized the ArrayList in the constructor since that seems more of a standard practice.

Perhaps you could have put an empty space from imports that are from different packages?

This method seems a bit too long. Perhaps you could have divided it into smaller methods?

Perhaps you could have used smaller helper methods to complete this method? Since there are big chunks of code that can be difficult to follow.

Perhaps you could name the method writeTasksToFile instead of writeTasksToFIle? This seems like a minor error in naming.

Perhaps you could have named the method findTasks instead of find? Since when you are using it in a different class, it might be difficult to understand what the method does, especially when there is a lot of code.

Perhaps you could have named the class TodoTask or just Todo instead of ToDos?

Perhaps this variable name could sound more boolean?

Might be better to add a javadoc for this class?

Perhaps this method name could be more descriptive?

I like how the code has followed the java coding standard.

Perhaps there could be a javadoc for this class?

Perhaps this variable could be made to sound more boolean?

I like how the function name describes what the code does.

I like how the variable names are not misleading.

I like how the function name uses verbs, as per the java coding standard.

I like how the variable names use standard words with no slangs.

Using the wildcard import is discouraged in this module. Maybe you can consider importing the relevant features individually?

I've noticed that this file is very long. You may want to consider putting different classes in separate files.

The variable "inst" is not clear to me. Perhaps use a clearer variable name?

This method feels a bit long to me. Maybe you can consider using helper methods or dividing this method into several smaller methods?

I think the variable "fileName" may be clearer here than "fname".

This method seems a bit long. Perhaps you can use helper methods to make the code shorter?

I've noticed that you have implemented quite a lot of subclasses that inherit from DukeException. Since the only difference between these subclasses are the error message printed, perhaps they can be combined into one subclass? (Together with a new attribute for displaying the error message)

Since any task must be either a todo, a deadline or an event, maybe you can consider making this an abstract class?

Personally I don't feel that fw is a very clear acronym. Maybe a clearer variable name here?

The description for the @param could be clearer. I've noticed the same issue in some other places as well.

I think that using switch and case would be a lot more readable than if else statements here

May I also suggest switching to a more OOP-oriented code. Perhaps you could create a Task object and extend it from there for each of the tasks and allow each task to handle its own inputList?

Maybe you could break this line into separate lines for readability? I noticed lines longer than 100 chars in several other areas too...

Perhaps these could be formatted as constants instead and be renamed using capital spelling?

I think this line "Pai Kia Bot: no description leh, try again" could be formatted as a String constant at the start of your code so that it can be reused throughout your program

Maybe this comment could be updated to be "blank" instead of "X" symbols

Is there any reason why Test methods have underscores for naming convention as compared to regular methods?

Not sure what "Ack" stands for

Maybe could rename to get TaskListSize() to be more precise?

Maybe could rename to findTask() to keep in convention with the rest of your methods?

Java coding standards stated that the imported classes should always be listed explicitly. I understand why you have imported * from duke.command package, but to comply with the guideline, would it be better to import explicitly instead?

Do note that there should be an empty line between the description and the parameters. I noticed the same issue in your other JavaDoc comments as well. Perhaps consider configuring your IDE to help with this styling?

This line has exceeded the limit of 120 words. Would it be better to separate it into 2 lines instead?

Do note that there should be no indentation for case clauses as to adhere to the Java coding standard. I noticed the same issue in the other classes as well. Perhaps consider configuring your IDE to help with this styling?

Despite not really violating the coding standards as it only mentioned that variable shouldn't be public, but since other classes or sub-classes are not accessing the keyword variable, would it be better to declare it as private?

Not explicitly stated in the Java coding standard but would it be neater and consistent if there is a breakline between package and import?

Maybe the method name can be changed slightly to explain the method more intuitively? E.g. processInputToAction? There should be a space between ) and {. I noticed this on your other methods and some of the if statements as well.

Perhaps a more meaningful name can be given to this variable in order to avoid confusion, and preferably in plural form since it is a String collection? E.g. splitStrings

Would it be better if it was isEnd? For making a boolean variable "sounds like" boolean.

    private boolean isEnd;

Java coding standard stated that: Single statement conditionals should still be wrapped by curly brackets. Perhaps you could tweak it a little bit? I noticed the same issue on some of the other if-else statements as well.

Distinguish clearly between single-valued and multi-valued variables. Use plural instead

check the indentation for the codes inside the try-catch statement

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

The ordering of import statements must be consistent. Maybe add a new line to group them

The ordering of import statements must be consistent. Maybe add a new line to group them

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.

Maybe you could split them into 3 different lines to make the code easier to understand

could try splitting this up into shorter parts so that the code isnt too nested with too many try catch blocks

you could consider using a setter to set the attributes of the task

should start the + in a new line

i would suggest writing writer.close() in a new line

method could be rephrased to sound more like a boolean method. Perhaps isAcceptedCommand?

Could have thrown the exceptions too

comma should be in the successive line instead of the end of a line

perhaps you could try using enum. Or since they are constants, let acceptedCommands be in caps like ACCEPTED_COMMANDS?

Could have considered the param to be named as taskIndex also to match the class attribute

Perhaps you could have a helper function to avoid deep nesting for better readability 😃

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-deep-nesting

Perhaps savedTask would be better as it's a singular File instead of a List of Files

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#use-nouns-for-things-and-verbs-for-actions

Perhaps you could consider having a helper function here too to make it easier to read. Ternary is honestly great and I get an idea of what you're doing though

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-complicated-expressions

Kind of a long statement here as well, perhaps I would suggest using StringBuilder for better readability 😃

Perhaps I could suggest declaring a static final variable to define what 2 is supposed to be here, I believe it's to ensure that there are sufficient arguments but maybe you could name it something like MIN_VARS 😃

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-magic-numbers

Perhaps you could have a quick description of this class since it's public

Maybe a quick description of the public class here would be good as well

Maybe a quick description of the public class here would be good as well

Apparently it's discouraged to use public class unless it's a class that's purely for data

https://se-education.org/guides/conventions/java/intermediate.html#variables

Same as above on the class being public

Really like the idea of the use of an interface where you define a template for the classes to follow. 👍

Shutsdown -> Shuts down

I think there has to be a new line spacing after a description for the params.

I like the idea of serializing the tasks. I am thinking whether would it be good if we can let a separate class to handle the string manipulation, such that the tasklist is only responsible for adding/deleting tasks. Just my thoughts! 😄

This is really good. I see that you are thinking of quite a number of scenarios where the tasks are being parsed in and checking their respective output. Keep it up!

Parse -> Parses

I am thinking for this part, perhaps we can give it to the Parser to do? Just my thoughts 😃

Do you think is it possible to break this code into 2? Makes it easier to troubleshoot and for future reuse. 😃

I see the parseDeadline and parseEvent has quite a number of similarities, do you think can these 2 be placed under one method instead? 🤔

Maybe instead of 'One' kind can be 'A'? Just my thoughts 😄

need -> needs

I feel the load method could be named more specific such as defining it as loadTasks, in case if the Storage method has multiple load commands in the future. Just my thoughts 😃

I think you want to put "ind" here as indentation. But i think it should be more expressive because it's attribute. Can be "indentation".

Also, i do not think that Command class is something that deals with Ui process, cause for this class you are using indentation and line as its attributes, which sounds not like a Command. Just my opinion.

Plural form also

attribute name should be "taskList" to demonstrate correctly what it means for. Tasks are separate tasks that maybe not related to each other, while taskList is different.

this attribute name "isConditionLocalDate" is not expressive i think. As i read this, I cannot tell what is the meaning of this attribute. Is it because of the order of the words.

Here the usage of indentation and line seems to be more correct and sensible. However, as i recall, it should not be named "line" and "line2". I mean there is number in it. Should be something without number and more expressive.

For me, I think the spacing between consecutive methods is good enough. I just think that at the beginning of the class ( between attributes and constructor) you should leave a one line spacing for easier looking

Do we need COMMAND_WORD and MESSAGE_USAGE here at the same time both serving as attributes. I think you should only keep COMMAND_WORD as attribute and the other could be converted to method return.

Hmmm i also agree with ZhangAnli this point. and deadlineDate seems good enough.

Also agree 😃) Just for me to add 1 comment. TY

Final variables should be written using uppercase.

Perhaps you can be slightly more descriptive here and explain what exactly is being saved.

Perhaps you can rename this to isFileAlreadyPresent to make it more clear what exactly the boolean checks for.

I feel that you can rename ret to something more descriptive which clearly indicates what the array list is supposed to hold.

Icon field should be in uppercase since it is a constant.

I like how you have inserted a line break here since the line is quite long but the line break should be placed after the comma.

Perhaps you can replace scan with scans to ensure that it follows the java coding standard?

Perhaps you can name this as isLengthFour to make it sound more like a boolean.

You can name the other boolean variables in a similar manner.

I think you can include @throws IOException in the Javadoc comment.

should be deadlineTest()?

Small thing, but should the tab spacings be 4 space wide?

Similarlly, should each tab spacing be 4 spaces wide?

Once again, should the tab spacings be 4 spaces wide?

Here again too, should the tab spacings be 4 spaces wide?

Here too, should the tab spacings be 4 spaces wide?

For this class too, should the tab spacings be 4 spaces wide?

Similarly here too, should the spacings be 4 spaces wide?

switch statements: should each case statement be at the same indentation as switch (i.e no tab before case)?

Similarly, should the case statement be the same indentation as switch statement?

Switch case might look better here?

Perhaps a more intuitive or descriptive variable name could be used here?

Perhaps a more intuitive or descriptive variable name could be used here as well?

Perhaps you could add a line in between these imports?

Should there be a line separating the imports and your class?

Perhaps a more intuitive variable name here?

Similar idea seen in the event task class later.

I agree, this will help streamline the Parser's job and respect the abstraction barrier.

Should haveWord be hasWord instead? This way it would sound more like a boolean method.

Should it be createToDo instead? Considering you used ToDoTask in line 142.

Are these breaks needed? I've seen them elsewhere as well. If you are not exceeding the hard 120 character limit maybe these could be in a single line to be a bit more readable? Also the next line should be 8 spaces from the original if you intend to break up a long line.

Hey, I like how clean this bit of code is!

Perhaps, you could list the classes imported explicitly.

Perhaps, the method name can be isIndexWithinRange().

Please leave an empty line between method summary and the parameters section. I noticed this issue in several other places too.

Please initialize variables where you declare them.

Please explain when the exception will be thrown.

Hi, I like how clean this bit of code looks!

Is there a way you could modularize this further to avoid having a large block of code?

Please leave a line between the method description and the parameters section.

Please follow the Javadoc format and specify the parameters as well.

Perhaps you could include an empty line between description and parameter section to comply with the coding standards?

Perhaps you could include an empty line between description and parameter section to comply with the coding standards?

Perhaps you could include an empty line between description and parameter section to comply with the coding standards? I noticed this in several other places as well.

Perhaps you could provide a descriptive header for this method, as well as other public methods?

Perhaps you could include a descriptive header for public classes?

Perhaps change the description from "list..." to "lists..." for consistency with the rest of your descriptions.

Perhaps change the description from "delete..." to "deletes..." for consistency with the rest of your descriptions.

Perhaps change the description from "list..." to "lists..." for consistency with the rest of your descriptions.

Perhaps rename str to something more descriptive like stringOfFoundTasks.

Perhaps rename str to something more descriptive like stringOfAllTasks.

Perhaps this variable could be named using a noun instead?

Perhaps this method could be named getTaskNumber, which is a verb, to better represent the method?

Perhaps you could be more specific as to which description is it for?

Perhaps specify filePath and folderPath instead? Otherwise it might sound misleading.

Perhaps spell out database? DB could be confusing.

I like how your method names in general uses verb, and accurately represents their functionality.

I like how the method names accurately represent their functionality.

Perhaps this variable could be more specifically named? I noticed the same issue under the Events class as well.

I like how you consistently place breaks before operators to improve readability throughout your entire code.

I like how the boolean method is named to sound like booleans.

Should have an empty line between description and parameter section

I like that the variables all have camelCase format

Space here as well

Space here as well

Can have javadoc comment for the method here?

I like that the method names all start with a verb

Maybe can have a more descriptive name than by for LocalDate?

Maybe a more descriptive name than at?

checkValidDate might be a better name, start method names with a verb

checkNumeric might be better here as well

Perhaps naming the method as printLine() since it is not operating on a collection of objects (not lines but rather only one line) and use verb at the start also?

Perhaps using a switch statement will be clear and easier to maintain?

Perhaps avoid using the wild imports and list imported classes explicitly?

Perhaps naming "end" as a more descriptive name such endDate ?

nice description of what the if condition will filter out with examples

Perhaps explain a bit more about the specific use of return since every other cases are using break?

Nice use of enums

Nice use of Optional along with orElseThrow

Maybe put a bracket around line.get(1) == "1" to make it easier to be understood

Perhaps use delete as present tense for verb?

Constant names must be all uppercase using underscore to separate words.

Deep nesting (4 levels), should avoid nesting over 3 levels.

Deep nesting (4 levels), should avoid nesting over 3 levels.

Deep nesting (4 levels), should avoid nesting over 3 levels.

Names representing methods must be verbs.

Names representing methods must be verbs.

Names representing methods must be verbs.

Write descriptive header comments for all public classes/methods.

Write descriptive header comments for all public classes/methods

Write descriptive header comments for all public classes/methods

Perhaps a less ambiguous variable name? If checked means done, then perhaps naming it isDone would be clearer

Maybe a plural form to represent the collection of Tasks?

Also perhaps a more intuitive name for a Task, instead of a CheckList, which might give the impression that it is a collection of objects?

See above for name suggestion

Perhaps a variable name that reflects the things contained in the list, e.g. tasks?

Perhaps a less misleading method name? It seems to be saying that it lists out the tasks in a string (what string?)

Maybe "tasksToString()"?

Perhaps a less confusing method name? e.g. showMatchingTasks?

Maybe write it as checkDateEquality?

Though iirc LocalDate has an equals method already

https://beginnersbook.com/2017/10/java-localdate-equals/

Interesting way of parsing which task it is! 😃

Perhaps a forgotten access modifier?

Maybe can consider breaking the line to enhance readability?

Maybe can rename to checkIfValidCommand? So as to make the method more verb-like?

Maybe can refactor this so as to have less deeply nested code?

Maybe can consider breaking up into smaller functions to help parse the input and check if the input is valid?

if line is a static constant in UserInterface maybe can consider naming it LINE instead?

maybe can consider putting line breaks here to enhance code readability?

maybe could consider not using static import here?

Maybe can consider reordering import statements in alphabetical order?

maybe can consider putting the comments on a seperate line before the if block?

Can the scope of temp be smaller?

Should this be named tempTasks instead?

I noticed there are several instances of this issue.

Can this be moved and reframed as a Javadoc comment for this method?

Is there an extra space between 'temp' and 'The'?

Additionally, is there a reason to name it 'temp' instead of 'task'?

This seems to function like an ArrayList with an extra setDone method. Would it be better to implement TaskList as an extension of ArrayList?

It looks like you are initializing temp separately for todo, deadline and event so you could consider declaring it within those blocks.

Should there be a description here?

Should there be an empty line after this?

I noticed there are several instances of this issue.

Should there be a full stop at the end of this line?

I noticed there are several instances of this issue.

I like how descriptive the Javadoc is! Should the first sentence of your Javadoc comments be a short summary of the method?

Should there be a header comment for each class?

Should the extra empty line between description and parameters be removed?

I like this! Should the line breaks come before the "+" operator?

Should there be a space after "if"?

Perhaps the names "start" and "end" can be ambiguous as they can be a verb or noun, so maybe they can be renamed to be more descriptive?

Maybe the extra asterisk should be removed?

I like how the method name is descriptive!

Maybe there should be a "break" or Fallthrough comment after the statements in case and default?

Perhaps there should be whitespace on the left and right of the "-" sign? (:

list.get(i - 1);

Perhaps imported classes should be listed explicitly? (:

Perhaps the method names could be more descriptive? In verb form? (:

Perhaps more javadoc comments for the methods? (:

In method header comments, the first sentence should start in the form "Saves ...", "Returns...", "Adds..." and not "Save...", "Return...", "Add..." (:

deadlineDetails (: perhaps a small spelling error

Perhaps convertStringListToString? (:

I like how you handled the exceptions for a lot of different cases! (:

Perhaps convertTaskToString? (:

Constant names must be all uppercase.

There should be a line break to separate functions. JavaDoc for public functions need to be added also.

Cannot write everything in main function. Even if it's not main function, this function itself is too long also, which reduce the readability of the code.

There shouldn't be a line break between JavaDoc and function header.

Agree with you, since the scope of arr is quite big.

Should JavaDoc start with an upper case letter? (Adds a task...)

Maybe should rename it as "addTask"? Besides, this function seems a bit too long.

Most of the functions in this class act as a printer. Hence, maybe these functions should be renamed as e.g. "printAddedTask"? Otherwise, it's not easy to know "addedTask" (and other functions in this class) are actually printers.

Constant names must be all uppercase. --> RESULT? There are also other constant names need to be change in your code.

I really like the way how you use switch. It makes the code elegant.

Maybe it's better to split into 2 lines.

Excellent coding style and exception handling.

Really like the way you add comments to make the code easier to read.

Same question

Just curious whether using absolute path will influence future distribution of the program.

Maybe it's better to check the taskType, for example:

'if (taskType == TODO) {

.....} else if (taskType == DEADLINE) {

.....} else if (taskType == EVENT) {

.....} else {

throw new DukeException(....);

}

'

Perhaps it's better not to use too much else statement as there could be some other types of tasks in the future and so it's harder to maintain the code. Maybe use more else if to judge the type of tasks will be better.

Maybe you can consider to change Process into Processes. Just one tiny suggestion.

Perhaps the method name could be changed to isFileAvailable() to make is sound more like a boolean method.

Perhaps the method name could be changed to isDirectoryAvailable() to make is sound more like a boolean method.

Perhaps the class name can be changed to ToDo instead.

A suggestion would be to use switch statements instead of if else blocks as it would make your code look a alot neater.

Method name should be in camel case.

Perhaps you can write a comment to describe why the catch block returns nothing.

Maybe you can give more description on what the easter egg class does

An explanation on why you are creating new objects instead of altering the current object would be great!

instead of checkInvalidTaskNumber, perhaps you can rename is to isInvalidTaskNumber.

Avoid using magic numbers as check conditions like arr.length != 4. If there is really a need, maybe you can use assigned the value of 4 to a variable in a way that can help increase code readability.

In the Parser.java, the executeCommand method consists of more than 50 lines of code. One piece of advice I will like to share with you is to try to avoid long methods and abide closely by the 30 lines of code rule for a method.

Distinguish clearly between single-valued and multi-valued variables. If possible, you can name the list into other more meaningful and obvious names like tasks.

Try to avoid misleading names such as isAlive because as a developer reading your code, it is easy to misunderstand the meaning behind the TaskList attribute, isAlive.

Do note that the catch statement should start on the same line as the closing braces of try block.

To add on what internityz has mentioned above, for the line 53 code in Storage.java, you can consider renaming the Arraylist>Task> taskList to tasks.

One thing I want to highlight here that can be of interest to you is according to the coding standard, each switch statement should include a default statement even if the default does not contain any code.

Do note that the else statement block should also start on the same line as the closing braces of the if block.

Just a small mistake here of having an extra line of space on line 91 in Parser.java.

I like how the imports have consistent ordering.

Might be better to add javadoc for this method?

Should this variable name be in plural?

Might be better to add javadoc for this class?

I like how the code has followed the java coding standard.

I like how the function names explain what the code does.

I like how the variable names are not misleading.

I like how the function name uses verbs, following the code quality guideline.

I like how the variable name uses standard words and no slangs.

From an object point of view, might it not be a bit odd to create a new Parser object for every input the user makes?

Consider renaming this variable to further specify what the contents of the list are?

Consider storing these numbers as static variables?

Consider storing these numbers as static variables?

Perhaps consider using the + operator for better readability? (As of Java 8 and onwards I believe it's converted by the compiler, so you shouldn't lose out on performance.)

Perhaps consider using the + operator for better readability? (As of Java 8 and onwards I believe it's converted by the compiler, so you shouldn't lose out on performance.)

Should this be in PascalCase (Ui)?

I like how clean this method is!

Perhaps this variable could be given a clearer name? It took me a bit to realize what it was meant to store.

Appreciate the effort to bring this statement to the next line because the line is pretty long. Makes it easier to read, keep it up!

Since this method is a getter used to fetch the value of isExit variable, should you consider changing this to getIsExit() or method names along those lines to avoid confusions between the class variable and class method?

Did you miss out a space between "Command" and "{" character?

As a getter used to fetch the numerical size of the task list, should you rename the method to "getTaskLength" or "getTaskSize" instead?

Should the class name be Deadline instead of AddDeadline? The latter sounds more like what we would usually name a method. This also applies for some of the other classes you have.

According to the Java coding standard for boolean variables/methods, could it be more appropriate to use isExit()?

Should final variables should be in all capital letters, including an underscore if there are spaces present?

I like how you organized this neatly because it is a long statement. Makes it really easy to read, keep it up!

Since this method is a getter used to fetch the value of isDone variable, should you be naming it as getIsDone() or any method name along those lines to avoid confusions between the class variable and class method?

Perhaps a more meaningful name for @param could be possibly replaced here

Not too sure if "Overriden" is the appropriate term here. I think "Overloaded constructor" may be more appropriate.

Minor typo for "list"

Might be a good idea to add "@Override" tag if this method is truly overriden

Perhaps consider standardizing on having or omitting "\n" between JavaDoc description and @param etc.

Second this. I believe your switch-case statements in your Parser were indented as per Java coding standards.

Perhaps the comment could be written as a JavaDoc instead!

Consider utilising other tags like @author

Overall, the code quality was great!, besides a couple of minute details, and I discovered different encapsulations and OOP use cases from your project. Keep up the good work!

Although line does not exceed the 120 character limit, perhaps you could add line breaks to improve readability?

I understand that Collections also use "size()". But the coding standard specifies that methods must be verbs. Maybe you can rename it as "getSize()" instead?

Similar to TaskList.size(), perhaps it is more appropriate to use a verb to describe this method?

Should there be a spacing between the header line and the parameter description line?

Perhaps using isNewToken and isUnmatchedQuote would better describe your boolean variables?

Perhaps you could redefine your logic here to avoid deep nesting?

Edit: referring to the process() method, not only these few lines. Sorry for any confusion.

Perhaps you could consider simplifying the logic within this block to improve readability? Also, is there a way to avoid performing multiple type checking and type casting?

Setter methods should be named with prefix "set", perhaps you should name it as "setComplete()" or "setDone()"?

Maybe you can consider setting this variable to "private" for encapsulation? Also, there are no classes inheriting from TaskList that would require access to this variable, so it should not be protected?

Perhaps you can also rename tasklist to tasks? I think it is more natural to have TaskList having tasks rather than TaskList having tasklist.

Do remember to leave a blank line between package and import.

For JavaDocs, your first sentence should start with "Uses" instead of "Used". Also, do remember to leave a blank line between your description and return values/parameters.

Do take note of the Java standard layout of import statements.

An example is as follows:

import static org.junit.Assert.assertEquals;

import static org.junit.Assert.assertTrue;

import java.io.File;

import java.io.IOException;

import javax.xml.bind.JAXBContext;

import javax.xml.bind.JAXBException;

import org.loadui.testfx.GuiTest;

import org.testfx.api.FxToolkit;

import com.google.common.io.Files;

import javafx.geometry.Bounds;

import javafx.geometry.Point2D;

import junit.framework.AssertionFailedError;

Shouldn't method names be verbs? A suggestion would be "readNextLine()".

Is this dead code? If it is, try not to leave this within your working code. If you think that you might rely on it again, make use of revision control to recover it.

Shouldn't this be "greet()" instead of "Greet()"?

Do try to follow the Java standard layout of import statements. Like static imports should be placed above, and there should be blank lines between imports from different packages.

Do try to avoid leaving empty catch blocks, or at least try to leave a comment explaining why it was left out.

Do try to give a meaningful names to class variables. As for this case, a suggestion would be to use "string" or "response" instead of "s".

Case blocks for "deadline" and "event" look quite long. Maybe abstracting out the code segments for handling input description and datetime could help to reduce length of case block and also place the code segments under method names that can be easily found if further editing is needed?

I noticed a lot of command.length() checkers in if statements across many case blocks. Maybe abstracting them out into a single method could help reduce indentation levels and case block length?

Shouldn't the classes imported from java.io be spelt out rather than lumping them together into a wildcard?

Personally, I prefer moving everything inside the for loop into a single method so as to reduce indentation levels. I believe it's worth some consideration, especially if you wish to convert this for loop as well as others into Streams.

Shouldn't the last line of the Javadoc comment be right before the Command class, rather than have a line space in between both?

Perhaps an empty line between the description and parameters would fulfil the code's adherence to standards regarding Javadoc? ... I've also noticed the same trait across many Javadoc comments in most classes

Shouldn't there be Javadoc comments on the CommandDelete class and its public methods? ...I've also noticed a few other classes which are missing Javadoc comments.

I also noticed that many one-line public methods from this point on also lack Javadoc comments. I believe that there should still be Javadoc for one-line methods to explain what the method intends to achieve.

Perhaps this unused method should be removed?

Would it be clearer if a named constant or method was used to extract the task number instead? (instead of using a "magic" number)

I feel that the code is clear enough for certain comments to be omitted!

Perhaps use a method to reduce the code duplication here?

According to convention and for better readability, maybe consider indenting the trailing comments to the same tab setting?

I think there are times when it is not really necessary to have multiple blank lines in between parts of your code?

Perhaps the code can be more readable if the IF/ELSE statements are written in a way that is less nested?

I feel that the use of line breaks can be more consistent! For example, whether or not to include an empty line at the EOF, or how many of such empty lines to put, etc.

Maybe consider deleting dead code? So that your codebase can be simpler and easier to understand.

Maybe "date" would be a better name than "event"?

Perhaps this check could be made into a single method? I see it repeated a couple of times.

Maybe use the "inform" like you did earlier? It may be good to denote the method just returning a string and not actually adding the task.

There might be a few unneeded spaces here.

The indentations could perhaps be more uniform here.

Perhaps it could be nicer to read without the extra line-breaks between the cases?

I like the use of "at" and "by" to denote the time for the different event types!

I like your use of line breaks to separate code blocks and comments! But for smaller methods, maybe it might be a bit distracting?

Perhaps there can be Javadoc comments to tell readers how this method works.

Perhaps it would be clearer if this method name starts with a verb?

Perhaps the parameter name can be something like commands? lst may sound a bit unclear to readers.

Maybe you can consider putting classes into packages.

Perhaps this variable can be named in the plural form.

Perhaps there could be Javadoc comments for this constructor.

Perhaps giving a more meaningful name can be better?

Perhaps it would be more comprehensive if this method name starts with a verb.

I noticed the same issue in several other places too.

Avoid complicated expressions. You can consider setting intermediate values first before calculating the final value.

You can consider setting parsedDate.format(DateTimeFormatter.ofPattern("E, MMM d yyyy"))> as a variable to improve readability.

I think there are too many else if statements. You can try restructuring the code to make it as unindented as possible.

Try adding javadocs, I realized you have not done it.

You can add what this code returns with @return. You can do this for the other methods too.

I think this could be more elaborate

I like how you personalise your code

This could be renamed to taskSize() since there is already a method called size() in java.

According to code standard, I think leaving a line break between the description and parameter section will be better.

Perhaps a more intuitive variable name here?

Perhaps it may be more concise to replace if-else if method with switch method here.

The Duke.java seems too long, will it be better to extracte some code out as some classes or functions?

Perhaps it is better to break before an operator "+"

There is a lot of the same indentation, maybe it can be extracted into a variable, which can be changed easily later？

Perhaps use variable name isRelated is more sounded boolean.

I like your usage of enum

Stating class variables as null seems redundant as removing " = null " gives the same effect. Not really a coding violation but lesser code is written as a result.

Perhaps it is better to have a longer method name here e.g getTask ?

getListToString seems more appropriate here as you are returning a String as the output.

The methods here should be renamed to reflect the actual use of the function. No printing is done within the method. e.g getWelcomeMessage(), getByeMessage()

Extra line. Might be redundant.

Same goes for here.

addTodo, addEvent and AddDeadline seems could be simplified by using inheritance, using a Base class instead.

Hard to recognise the use of the function from the name at first glance. Perhaps toSaveFileString()?

String des could be renamed more meaningfully as description?

makeDeadline() >> createDeadline()

rename method name?

rename st to storage?

try "tasks", "lists" instead of "lst" ?

Empty line between description and parameter section?

Empty line between description and parameter section?

Consider using switch case statement, better readability?

Consider using switch case statements, better readability?

Could this method be separated into multiple shorter methods?

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-long-methods

Would it be possible to simplify these expressions?

https://nus-cs2103-ay2021s2.github.io/website/se-book-adapted/chapters/codeQuality.html#avoid-complicated-expressions

Would i be possible to shorten this method such that it is shorter than 1 page?

Would it better if you imported only what you needed?

Would it be better to use SCREAMING_SNAKE_STYLE for static variables?

Would it be better to use SCREAMING_SNAKE_STYLE for static variables?

Should there by a period at the end of the param description?

Would it be better for this method be compartmentalized into smaller methods that are shorter than 30 LOC?

Should list out all the import class instead of using wildcard.

Maybe a empty line between the description and @param would make the comment more similar to the coding standard for comments.

Maybe remove some empty lines here to make the code more concise.

Maybe remove some empty lines here so that there is no big empty space at the end of the code.

The name taskNo could imply not having task instead of task number.

Maybe put an empty line between the description and @param here to make the comment more similar to the coding standard for comment.

Maybe the term CSV in the method name could be Csv instead.

Maybe the boolean name could be isDone instead of done.

should there be a variable name that is more intuitive at indicating that the variable is a multi-variable?

The coding standard suggests that the Iterator variable (t) should only be used in nested loops only,

use Nouns for Things

Perhaps a more intuitive variable name here?

Perhaps it better to put "_" between the word as suggested in coding standard for better readability. (PATH_NAME instead of PATHNAME)

In method header comments, the first sentence should start in the form Returns ..., Sends ..., Adds ... (not Return or Returnning etc.) : from https://se-education.org/guides/conventions/java/intermediate.html#comments.

... I noticed the same issue in several other places too

Maybe the JavaDocs header comment for the enum class is missing?

I think JavaDocs comment is missing for this class.

I think JavaDocs comment is missing for this class.

Should there be a JavaDoc here to tell us about the functionalities of these methods?

Should this line be in Egyptian style?

I like the naming of these methods, it really shows what the methods do. However, write a JavaDoc comment might be able to provide more information about the method.

Should this IF-ELSE statement be in Egyptian style? I noticed the same issue in other classes as well, do note that according to the Java Coding Standard, IF-ELSE statement should follow Egyptian style even though it is only a one line code.

Should there be a JavaDoc Comment to tell use what does this class represent?

I like that you use "user" and "duke" to differentiate the respective images. However, would it be more appropriate to name them as "userImage" and "dukeImage"? As "user" and "duke" might be a more suitable name for the respective Object Class.

Should there be JavaDoc comment to tell use what the class represents? I noticed the same issue in several other classes too. It would be good to have a JavaDoc comments on the class so that we can know more information about this class.

Should the switch-case statement have the same indentation?

Perhaps use a more descriptive name for "first"

Perhaps you could add a line break to improve readability

Maybe declare a constant for "\u2713" & "\u2718"

Maybe change the method name as it is currently printing all the items in tasks

Perhaps you could change the variable name from taskList to tasks. Same for the other methods

Perhaps you could explain the return values

Maybe you could add comments for both the add method

perhaps a more descriptive name for tokens[0]

I think from first glance, it could be useful if you described in greater detail what you mean by "an object of the Command type", but overall pretty self-explanatory method names. 👍

I like that it throws the standard IOException for errors, it helps that it's not misleading, as @yeoutzer said!

Slight typo for as, but its pretty good overall! Keeps to the specified coding style!

Could use a space after Task, but perfect otherwise!

It'd be nice to leave a Javadoc header for the DialogueBox class!

I like that the error thrown is very self explanatory!

Some of the methods, though already very self descriptive, could use some javadoc comments as well.

Could use some javadocs!

Perhaps you can use your Keyword enum here since the cases are your Keyword enum values?

Perhaps the code inside the two if statements can be abstracted into another method? The code is almost the same in both if statements, with the exception that one is for creation of Deadline while the other is for creation of Event.

This exact code which parses a String to Date object can be seen multiple times throughout your code.

Perhaps would it be better if the method is abstracted into another class (eg. DateParser) and made into a static method?

This way, you can still access the same code from your other classes and at the same time, improve the maintainability of your code as you would only be required to edit it once if it is abstracted into a separate method and class.

There is a missing line break between the variable declaration and constructor. You might have missed it out by accident.

Perhaps you can include an empty line between the description and parameter section in the Javadoc. It will help to improve readability and will comply with the coding standard.

There is a small indentation error at *. I believe you have missed it accidentally.

Perhaps you can include an empty line between the description and parameter section in the Javadoc. It will help to improve readability and will comply with the coding standard.

I have noticed this issue exists in all your other Javadoc too.

Perhaps you could use the three part format featureUnderTest_testScenario_expectedBehavior() for test method names? The three part format provides more clarity on what is being tested in the method.

This three part format naming convention can also be applied for your other test method names.

Should a header comment be added here?

Should a header comment be here?

Would a more descriptive variable name be more suitable since the function parameter has a large scope?

Issue mentioned in other places.

Possibly make it more succinct by changing to toAdd?

Possibly use a local variable name for the number of tasks in the task list?

Same comment as prior about local variable name.

Would matchedTasks seem better?

I liked this idea of having an UnknownCommand class

I like how it's easy to understand the purpose of this test from the method name, noticed it in other instances too.

I like how it's obvious how your UI formats messages from this method body.

Perhaps there could be a header comment for this class, if it's meant to be your main class? (If I assume right, this seems to be your entry point for your programme?)

I like how neat this if else block is. There seem to be some hardcoded strings passed into the split function - perhaps they could be assigned to some variables with descriptive names, or other comments could be added to explain more about what's happening in this function?

Could this variable be named to sound more 'boolean'? (I did like that you've followed boolean naming conventions in other places)

Agreed with this reviewer, it's easy to understand what errors are being expected especially with your javadoc comment too

Perhaps there could be a brief comment on what a fresh task means here?

Does the 1 here look a bit like a magic number? Perhaps can consider explaining it in the comment

Could the DateTimeFormat perhaps be saved as a final String to avoid repetition?

Interesting classes!

A final int TASK_DONE = 1 could be optionally used here

Should here be a empty line here?

Not sure if required but the JavaDoc comments all appear to end with fullstops

In method header comments, the first sentence should start in the form Returns ..., Sends ..., Adds ... (not Return or Returnning etc.)

-from the styling guide, perhaps can consider saying "processes the task...."

Nice emoji!

Should these have punctuations?

Possible to review the other Javadocs in the rest of your code too.

Should taskList be tasks instead?

Perhaps you can consider naming the Scanner object as a noun rather than a verb.

Do consider using plural forms for collections.

Agreed with IceBear. Would also make program easier to edit rather than having to Crtl-F the class every time.

Also, can consider explicitly listing which packages are being imported rather than using wildcards.

Not sure if this filepath will work on non Windows machines.

Should there be a line break between description and parameters section? Can also look at other Javadocs.

Should the name of the collection be in plural form instead?

Should you use a wildcard import (*) here? Would it be better if you listed out the needed packages only?

May I ask for the purpose of the spaces in front of your output string? Is this for aesthetic purposes? I have noticed this in a few other places as well.

I think it would be good to add a spacing right above the try block to separate the code logic between the two blocks.

Can I ask if the spacings in front of your string output was intended? This seems to be done on purpose and I have seen it quite often. Is it for UI purposes?

I believe checkstyle prefers if there is an additional empty line after the last parentheses in your files. You may want to check up on that.

Do you think it would help if you left spaces between consecutive methods? Have noticed this in a few places and because of the javadoc comments, it may make your code seem a bit "cramped".

Perhaps a more intuitive name for the localdate variable will be better? I feel that just stating "by" could be hard to read for some.

Would like to clarify the purpose of this file since it is essentially an empty file?

Might be better to use tasks as variable name, might confuse yourself with the TaskList class.

Perhaps following the featureUnderTest_testScenario_expectedBehavior() test method convention will be better.

Same as above, featureUnderTest_testScenario_expectedBehavior() convention.

Same as above, featureUnderTest_testScenario_expectedBehavior() convention.

I agree with @figo2127's comment! Might confuse yourself with the Deadline model name.

Nothing critical, whitespace after DukeException required 👍

Same as above, whitespace after DukeException

test method does not follow the featureUnderTest_testScenario_expectedBehavior() naming convention

Should there be some documentation for this method? I noticed there aren't any javadocs for other methods in the code as well

I think a //Fallthrough comment is needed here in case DEADLINE and EVENT according to the coding standard? Similar issue was spotted in Parser and UI.

Maybe a default case should be added here? I'm not 100% sure but the coding standard provided states that a switch case should have a certain form and it included a default case.

I like that this ternary was broken into two lines to keep within the 120 char limit but should it be formatted as

(CONDITION)

? method1()

: method2()

instead?

Could this expression be expanded? It looks a little difficult to decipher. (Textbook section)

Could these blocks be simplified / abstracted out? (Textbook section)

I like that all your method names are verbs where applicable

Would a switch case here make the code more readable? I don't think its in the coding standard so it might just be a personal opinion but the block of code here looks like it fits exactly what a switch case would do.

Perhaps a better way to name it would be "textInput"? Since there is a "cmd" or Command type that could be misleading.

I believe it would be better to name your variable fully ("tempString" instead of "tempStr") to improve readability!

Seeing that there are multiple if else cases, perhaps you may want to consider using a switch case? That would improve future developers to be able to figure out components easily by looking at the case conditions! 😃

Perhaps you may want to separate the classes into different Java files? That would certainly declutter the amount of code in single Java file and makes it easier for developers to look at a specific area of code. Doing this would cause future extensions to increase the number of lines in a single class even more.

Perhaps you may want to change this to Egyptian style braces (CS2103T coding style)?

Maybe you want to change the boolean variable name to sound like a boolean instead? How about "isTaskDone", how does that sound? 😄

Haha, I get the joke! But maybe it would be better to name it to a more readible and intuitive variable name, like "taskList". In case some developers do not appreciate the humor. 😝

I like your use of switch case statements! However, do note that we may need to add the "//Fallthrough" under each case where a break is supposed to be placed, but unreachable!

Perhaps might want to add more spaces in front to indent this method to fit with the Java coding convention.

It might be a better idea to import relevant files from the task folder instead of importing all?

I like how you remember to add spaces around i + 1;

"The explicit //Fallthrough comment should be included whenever there is a case statement without a break statement.", quoted from Java coding convection.

Perhaps you can change parserTest2/3/4 method name to something more meaningful?

Would be better to avoid using misleading names?

Perhaps can change it to showAddedMessage?

Might be a good idea to rename t ?

The variable name "by" is not clear and may lead to confusion sometimes.

Maybe naming it as isSaved or isSavedBefore make it sound more boolean?

I agree with Tomashiwa on this, pass the taskList into Parser from your TaskList class will make it simpler.

Variable naming should be more detailed to reduce confusion

Maybe using constructMessage sounds better and more fluent than using constructMsg?

This method doesn't serve to construct TaskList in my opinion, perhaps naming it as displayTaskList would sound more reasonable. What do you think

Maybe naming the variable in full will be clearer

This looks abit too nested and maybe have another helper function to reduce the blocks?

Maybe instead of using the else block to handle the case of an Event task, it can be used to handle invalid task types?

Should the block to handle to do tasks be stated explicitly in an else-if block instead?

Same comment as before.

Perhaps format this sentence such that it starts with "Returns..."?

Should there be a short summary of the method here?

Should case blocks have the same indentation as the switch statement?

Should case blocks have the same indentation as the switch statement?

Should plural form be used to represent a collection of objects for ArrayList>Task>?

Should there be an empty line between the description and parameters?

Should you try to avoid using wildcards when importing

Should you have an ordering of your import statements?

Should your boolean variable name be isDone?

Should your variable name curr be currentTask?

Should there be a punctuation at the end?

Should the parameters require some description?

Should there be some comments in this line for the return value?

Perhaps you could try to decrease the number of nested blocks.

I believe that savedata does not follow the coding standards, and it should be saveData instead.

I think there is a slight inconsistency with the coding standards in errorMsg and that the '+' should be in the second line in front of the "Please don't manually edit the save file.";

Similar to my above comment, I think you could try to reduce the amounts of nesting in the method parseTask.

Perhaps the instead of 'parts' the array name could be more descriptive.

This method could have been called getTask instead for more clarity.

Similar comments for this delete method and the add method below. I think deleteTask and addTask could be better method names.

Perhaps switch-case could be used instead of if-else statements.

Do you think 'private' would be a better modifier for variable 'by'?

Would it be better to name the boolean variable as "isDone" and the char variable as "status" instead?

Do you thinking creating a utility function to re-format String 'save' at one go will be helpful? It may make the method shorter and more concise:)

Would creating a String variable to store tasks.storage.get(i).getDescription() before the 'if' statement be better?

You may also consider creating:

Task task = tasks.storage.get(i)

String description = task.getDescription()

😃

Would getCommand or extractCommand be a better naming?

I've also noticed the same issue in other methods. For example, the 'description', 'date' methods. It might be a bit confusing without a verb in the method name 😃

Since it's an add command, do you think addTodo, addDeadline and addEvent are more informative naming?

I've also noticed the same issue for other Command child classes:) but I like the way you standardised similar method names across all child classes.

Will getSize() be better?

Perhaps it is better to name this method as isOnDay?

Perhaps you can consider choosing a more descriptive name for your parameters?

Maybe the starting letter can be capitalised?

Consider adding an empty line between the description and the parameter section? I noticed this minor issue in other areas of the code as well

Perhaps it would be good to add comments to some of your code logic such as tokens.length >5

Perhaps the extra spacing before "Input string." can be removed?

I like that you have a method to deal with string formatting. Maybe the method name should be joinStringWithNewLines instead? (ie Lines instead of line)

Maybe a short summary of the method could be written as the first sentence for this method header.

Plural form could be used for this String array to make the variable name sound more like a collection of string values.

Would it be better if javadoc comments were included as there is more than one constructor?

Is there a more descriptive variable name that could be better for 'at'?

It is recommended for the method header to start with 'passes' instead of 'passed'.

'isFound' may be a better alternative to 'found' as it is a boolean variable.

Well written error message as it is very descriptive and tells the user what to do.

Don't forget to private attributes for encapsulation 👍

Perhaps use singular form (ToDo) since plural form usually represents a collection of objects, eg. List

Very minor but remember to leave white space before "{" just for consistency and readability 😄

Perhaps could abstract the file type "T" out as a constant variable? same for other file types too!

Encouraged to use verbs for method names! Maybe example getFileString()?

Boolean name should sound like booleans. eg isDone

Remember to leave "Empty line between description and parameter section"!

Should there be an empty line between description and parameter section for javadocs? I noticed this for your other javadocs as well.

Should storage and ui be in uppercase?

Honestly, I wouldn't consider this to be violations, but I'll leave it to you to judge:

- Your variables are final whenever possible, which is good, 

- However, that means it probably should be written in uppercase according to the coding convention. 

- My personal belief is that only static final variables should be written in uppercase, but the coding convention isn't clear on that. 

- However, an example they provided was `final int COLOR_RED = 1;` which leans towards the former,

I would leave them as non-final to avoid meeting this problem.

Should storage and ui be in uppercase? I noticed this in other areas as well.

Honestly, I wouldn't consider this to be violations, but I'll leave it to you to judge:

• Your variables are final whenever possible, which is good,

• However, that means it probably should be written in uppercase according to the coding convention.

• My personal belief is that only static final variables should be written in uppercase, but the coding convention isn't clear on that.

• However, an example they provided was final int COLOR_RED = 1; which leans towards the former,

I would leave them as non-final to avoid this iffy-ness.

should pathname be pathName instead?

should taskList be written with a plural word instead? ie. tasks

not a coding standard violation, but List.remove() returns a Task, so you can actually do Task deletedTask = tasks.remove(index) instead 😃

This is the first time i'm seeing this syntax. How does this work?

Should a wildcard import be used here? Perhaps you can consider importing the specific class if possible!

Here instead of breaking after the operator you could break before it. For example, instead of:

"...." +

"...."

you can do:

"...."

+ "...."

Should the else if statement be on a different line than the curly bracket? Since you have used the Egyptian style braces for most of the project it might be a good idea to update these parts to make it consistent

Should be a quick fix but I noticed that the indentation here is off by 1 space!

I like the use of comments for these public methods, perhaps you can take it a step further and make them javadocs comments instead.

I like the small detail of adjusting the grammar based on how many tasks there are! At the same time, this ternary makes the expression slightly hard to read. Perhaps you could consider moving it out of the println.

Great error checking here, but instead of nesting the if statements, perhaps they could be combined into one?

Since this class indicates a singular Deadline Task object, it could be better if you named your class in the singular form instead of the plural form. This also reduces confusion should the need for having a Deadline array arise.

I have noticed that you have implemented DukeException.java as a error handling class. Perhaps you can leverage that to handle your exceptions instead of printing them here?

I like how thorough your error handling is. However, the main function is very long. Would it be better to abstract some if-block code into separate functions to aid readability?

Since this is a function that does an action, would it be better to name it in a way that it sounds like a function?

I like your use of the inherited toString() method.

Since this function is an action, would it be better to name this is a way that sounds like an action? (Eg: markAsDone())?

Since this is a function that does an action, would it be better to name the function as a action? (Eg: printAddedTaskReply) This would also apply to the below Reply functions as well.

Consider not using this unless the member variable is shadowed by a parameter. I think this coding standard is not stated in the Basic + Intermediate Rules page but only in the All Rules page of our Java style guide.

Consider ending your parameter descriptors with punctuation as specified in the coding standard.

Not sure I like the length of this method. Maybe abstraction could be applied here. For instance, the general format seems to be

if command matches some format, then parse the parameters, then execute the command, then return feedback

I feel that there are four responsibilities here:

1. matching the command

2. parsing the arguments

3. executing the command

4. building the feedback message

Perhaps each responsibility could be given to a different class or method.

Consider standardizing import orders (e.g. java/javafx packages first then duke packages) in this and some other files. I like the nicely separated imports in Duke.java.

Is the this qualifier necessary here? I noticed this in several other files as well.

Consider formatting this method using Egyptian style braces even though it has an empty body. I noticed this in another file as well.

Consider ending the param comment with a fullstop for consistency.

Maybe there should be an empty line between the method description and parameter section

Perhaps could add punctuation behind each parameter description

maybe can stick with } else { indentation, comment can be moved up

Perhaps an empty line between description and parameter section could be added.

Maybe could add punctuations to the Javadoc comments

Perhaps a line break could be added here.

Perhaps could remove the line break for the try-catch statement.

The parseInput method here is a bit long. Maybe its better to separate some of the parts like the deadline case into its own handleDeadline method for readability?

Maybe use a verb for the method here? Something like sayBye()

Good indenting! I think it can be more readable with the plus in the front of each new line instead of at the back of the previous one.

        String output = "++++++++++++++++++++++++++++++++++++\n" 

                + "Here are the tasks in your list: \n" 

                + "TaskType | isDone | taskName | time (if any)\n";

Maybe use isCompleted instead of completed?

Good formatting for switch cases!

Names of methods should start with a verb. So maybe here it can be checkIfNumeric?

Collections usually use plural form. So maybe tasks can replace taskList here.

You should check the switch identation

Avoid leaving spaces!

You should avoid overly long methods that take up more than the space in a computer screen

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line

Boolean variables/methods should be named to sound like booleans

Boolean variables/methods should be named to sound like booleans

Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line

The case statement should be at the same indentation level as switch statement

The indentation is a bit too excessive here, should only be 8 spaces from the line above

I think you can select a better variable name than t, as the scope here is for the whole method.

I think it is confusing to have the variable name the same as the method that is called. I suggest that you change the variable name(see above comment)

While isExit is a valid variable name, I think you can choose a better one like shouldExit or canExit. From an English readability point of view, it will help things slightly.

I like that you managed to encapsulate all your commands under a single Command class

There should be a space between the ) and the {

Hi, in terms of code quality, perhaps we can consider to use words that are more descriptive for these variables instead of arr and arr2.

Hi, similarly to the previous commit, perhaps the variable name for this could be something like "fileOutput" instead for better readablility.

Hi, according to the "Explain WHAT and WHY, not HOW" section on coding quality. They mention that comments explaining how the code works should be avoided. Maybe it will be better to remove this comment entirely as I believe the would understand the intent of the code nicely.

Hi, I notice that all of the constants that has been declared final is written in the camelCase format. Not sure if it should be written in uppercase using underscore to separate words, since the it is declared final and hence, a constant. I noticed the same issue in several place as well.

Hi, I am thinking that boolean methods might be more appropriate to choose a name such as canContinue to conform better to the coding standards.

Hi, breaking up this line could make your program more readable. According to the coding standards, it will be best to have line length >=120 (Hard Limit) and >110 (Soft Limit). The character for this line is around 118 so it might be better to break this line up instead.

Perhaps this could be commandDetail instead as it might not need to be represented in its plural form as it is not a list or array of objects.

Perhaps you could add a space between ) and {?

Perhaps you should add a comment explaining what this class is/does?

You may want to consider changing your test names to something more related to what is being tested. For instance

sortList_emptyList_exceptionThrown or getMember_memberNotFount_nullReturned

perhaps you could just shorten this conditional to

obj == this || obj instanceof ExitCommand

?

Consider splitting the imports

Perhaps you could add a comment on what this class is/does?

Perhaps expand the wildcard import instead?

A minor detail, but shouldn't there be an empty line between the method's description and parameters in the JavaDoc comment?

I noticed this in other parts of the code as well.

Shouldn't it be Launches instead of Launch?

Shouldn't this JavaDoc comment be updated?

I like how you have created an abstract Command class 👍.

Wouldn't it be more concise for the method name if it was addTask instead?

Shouldn't this JavaDoc comment include a description?

Shouldn't the names of the test methods be more descriptive? (e.g., by using the three part format)

consider enumerations instead of case and switch as I believe enumerations are not case sensitive

It would be a good idea to explain what the param id for i.e. explain what listener is for and what it does

the input names are obvious so good job there, but the explanation doesnt really add anything to the javadocs

I usually comment in the beginning what functions each import does

I read somewhere that its better to name these getter methods getterIsDone

instead of while(true) I believe we can also do for(;😉

perhaps make a variable for commands[0] = firstCommand for ease of reading

Perhaps since this is a getter, the method name could be getTaskSize() instead?

Maybe you could consider avoiding long methods as the textbook says to be wary of methods that go beyond 30 LOC? This could improve the readability of the code.

Perhaps renaming list as tasks would help to show the content of the list?

Appreciate the use of named constants to improve readability of code!

Maybe consider putting this in Ui class for consistency since you have already put everything related to printing in that class?

Maybe renaming the ArrayList as tasks would be more clear in showing the content of the list?

Appreciate how you bring long statements such as these to the next line! Makes it easier to read.

A clear and concise name for the method! Perhaps you could include a JavaDoc header for this method as well?

Good line breaking to improve readability

Constructor is well-defined in the header but the space in between the header and method can be removed to help improve readability

According to coding conventions, would it be better to write out the full definition of the method here?

The space here could be removed to help improve readability.

Would you consider removing "Msg" from the name to make it shorter?

Good use of abstract methods here!

A more meaningful naming suggestion: String input, Pattern pattern, Matcher matcher. I found this issue in the Storage class as well.

Can this method name be loadTasksFromStorage to explain the method more intuitively?

Should this method be private instead, because it is independent of the whole program and is only used by the load method from the same class as an encapsulation?

Would it be better to name this method as formatDateString to reduce confusion?

There should be an empty line between the description and the parameters here. I noticed the same issue in most of the other classes as well. Perhaps consider configuring your IDE to help with this styling?

Thoughtful addition of the redirecting links to Task in this bit of JavaDocs!

Java coding standards states that imported classes should always be listed explicitly, although I can see why you would import * for this package duke.command. Should the different command classes be encapsulated such that BYE, DELETE and DONE can result in calling of a single factory method?

Maybe it would be better if you avoid deep nesting to improve code quality

Maybe it would be better to leave out the indentation for case clauses

Would it be better to have this line format as a constant since it is repeated several times in the class?

I like your indentations in this class!

Would it be better to change parameter boolean done to isDone?

Maybe it would be better to change the method name to getSize() since it is a getter

Maybe it will be better to change doneTask to a verb such as checkTask

I like how your command classes are named clearly

Is it better to name this indexOfSlash?

Is it better to name this dateBy or dueDate?

I like it that you have added clear javadocs for public methods!

Hi, I agree with TanJin and I think that parsedInput makes sense because the program is handling one line of user input as once.

Would it be better to name this expectedMsg?

Would this method name be confusing for some readers?

Missing description of return String?

Not sure if it's just me, but I feel that this method looks very cluttered. Also, missing return definition? Not sure if it's intentional (or if it's allowed).

The name "at" doesn't seem very comprehensive and I wouldn't be able to tell what it is used for unless I know what an Event object is meant for and how to create one from the CLI. There is some discrepancy comparing this with your Deadline "by" property as it is of type LocalDateTime in Deadline whereas it's just a String here.

Similar to the Event class, I don't think that the name "by" is very comprehensive unless I know how to create a Deadline object in the CLI.

I'm not sure if this constructor serves a purpose as you'd be able to create a Storage object just fine without this. Unless you're setting it up for future modifications.

For all of your JavaDocs,

1. There should be punctuations at the end of every sentence in your description and every parameter definition. (Note that the descriptions are not supposed to be too long unless it is completely necessary)

2. There should be an empty line separating the description and parameter fields.

3. All sentences should have proper capitalisation.

4. JavaDocs of a method should be directly above a method, without a line separating them.

Below is a screenshot from the Java Coding Standard page in the "comments" section for your reference.

"print" should be used instead of "printing".

Good job on naming the interface correctly in PascalCase. I like your use of an interface here.

Hi, just a small inconsistency that I spotted. For some of your classes, there is an empty line between the class declaration and the class properties. However, some of the classes don't.

Perhaps it will be more consistent if all classes have an empty line in between or none of them have an empty line in between?

Perhaps a more descriptive name for the boolean variable (instead of "flag") will be better?

Hi, I suppose you could have done without super(message); and it will still work as intended?

Let me know if there is actually an intended effect of your super(message); 😃

Perhaps the use of String.format() could have been better in terms of understandability?

Perhaps extracting these magic numbers out as named constants will be easier to understand?

Overall, the process method seems to be longer than desired. Quoting our textbook:

Be wary when a method is longer than the computer screen, and take corrective action when it goes beyond 30 LOC (lines of code). The bigger the haystack, the harder it is to find a needle.

Perhaps extract out some smaller methods?

Imported classes should always be listed explicitly. I noticed the same issue in serval other places too.

Is there a better way to do this instead of using instanceof? Maybe implement a checking system.

Imported classes should always be listed explicitly.

To be honest im not too sure, but I think we were taught in 2030 that the use of instanceof breaks the open/close principle in some way you may refer to this StackOverflow page: https://stackoverflow.com/questions/20589590/why-not-use-instanceof-operator-in-oop-design

Please refer to the CS2103T coding conventions. Switch and case should be in the same indentation.

Consider packaging them under duke.task instead.

Consider using underscores in your test methods (refer to coding standards).

Hi, I think for this the case: statements should be aligned with the switch statement instead of a 4 spaces indentation.

Here also should align the switch statements!

Usually I think it’s better to use method names that are verbs rather!

Just a small typo I guess, getDateError()?

Again I guess should be listTask()?

This should also be userInput()

This should also be findTask

Whitespace before {

Whitespace before {. There are similar cases in other files as well, I will not point out all of them.

Using the '.*' form of import should be avoided.

Whitespace before operator '+'. Note there are whitespace issues for other keywords like 'for' in other files as well.

Missing javadoc for class Storage and TaskList.

Careful about the import statements ordering. Similar issues in other files, for example TaskList.java

Should the "bye" be converted to a static string BYE?

Could this be converted to a switch for readability?

Is it textWarper or textWrapper?

I like how you used a hashmap. Very clean. 👍

Should this be in caps?

e.g. private static final String LOGO = ...

Should these be made into a static variable e.g. BYE_COMMAND = "bye"?

I think you could change the name of this variable to make it more descriptive! Maybe something like "dueDate".

Since this is a constant variable, it should be named in caps!

Same as before, maybe could use a more descriptive variable name such as eventDateTime!

Same as before on constant names. Also I didn't mention previously but maybe could private the message since theres no need for any client to use this message outside of the class.

Small indentation issue!

Small indentation issue!

Spacing

Spacing

Shift the lines to make it top down (42 to 41)?

No wildcard imports

Can store the Task type as an attribute so you dun hard code, e.g., taskType = "T"

Try catch block?

Should a JavaDoc comment should be added for isNumber too?

Good choice of String thrown, really helpful for the user in an actual chatbot context 👍

This is a great touch, I didn't think of catching this exception so as to print a line informing users about the date format.

Agreed 👍

I believe the toString() method, even though overridden, needs a JavaDoc description still.

(Likewise for the other classes too)

I believe a line should be spaced between the description and the @param for all JavaDoc commentary (even though it is not explicitly stated in this mod's check requirements). The JavaFX files practice this too.

Minor issue, but you could add a period at the end of each parameter description.

Minor issue, but you could omit 'A method that' in the method summary.

Trivial but there is no need for a blank line between javadoc comments and the corresponding method.

Perhaps you could consider using switch-case here given there are a large number of cases (>5)?

I think this is trivial but perhaps you could initialize the variable tasks under class TaskList to new ArrayList()? Then you might be able to do away with the empty constructor.

For enum name PrintText, perhaps you could change it to something like ‘PrintedText’ to make it sound more like a noun?

Minor nitpick but perhaps consider starting Javadoc sentences with an uppercase letter and ending with a full stop to keep within the coding standard.

Perhaps int taskToDelete could be indexOfTaskToDelete even though it may be quite long.

It may help to distinguish it from Task toRemove.

Perhaps you could include Parser as an instance variable for the Duke class?

The same change mentioned above can be applied here as well.

It may be possible to use String userInput as an argument for this function instead of an instance variable for the Parser class.

That way one Parser object can parse multiple user inputs.

I am inclined to agree with guanyz here. If possible, hope you would consider some of the possible modifications stated here.

Would AL be a bit confusing here? Perhaps tasks or taskList would be better?

Since return also breaks out of the switch block and there are no fallthroughs here, would it be better if the // Fallthrough comments are removed?

I agree with vevek, arrayList or taskList could be better.

You may have missed out the Javadoc comments for getDescription() here.

No Javadoc for findMessage().

Public constructor here with no Javadoc.

Should we avoid using wildcard import?

Should we remove the blank lines here?

Should we remove these comments since it's not used as an attribute of the class or for documentation purposes?

Is the switch statement case indentation supposed be as such?

Should Delete method begin with small capital letter?

should Remark begin with a small capital letter as well?

Following the coding guidelines, method names should be verbs. Perhaps handleError() would be a better method name?

Since this method is a setter, maybe setStatusTrue() would be better? It would explain to readers what status you are changing to as well!

Perhaps adding class level Javadoc comments would aid readers in understanding the function of each class better!

Maybe you could specify why/when the exception would be thrown?

Should old commented-out code be removed to make the code cleaner?

Minor typo here!

Agreed! I noticed this in Storage, TaskList, Ui and Command classes too.

I like how you broke up this line to improve readability!

Perhaps renaming by to dueDate would be more descriptive? Same for the Event class.

I like the use of switch statements, makes it easier to read than a bunch of if-else blocks!

I like how you break up long lines like this to improve readability!

Maybe you can leave a line between import statements from different packages?

Could consider making this private.

I think it's supposed to be data/duke.txt

You can make the getParentFile first, then the actual file to do this. (or take a look at my Storage class)

And I think you should refrain from hardcoding "duke.txt" multiple times. 1 time should do.

Sorry to nit-pick. The noun greeting may be better.

I think these can be private, as with your other classes extending Task.

I think this is one way. But there are also ways to assert for exceptions. Alternatively you can simply

public void todoTest() throws DukeException()

I think

If I'm not wrong, I think we're supposed to edit our environment variables and not paste the full path here.

I remember seeing an forum issue, made by fairyinabottle, that helped me solve it.

I don't need a constructor for Parser especially when it does nothing.

I think you can remove the lines before each else if statements.

Example:

if() {

return;

} else {

return;

}

Instead of commenting out the codes that is not required, removing them will be better.

I like the way you use appropriate naming for strings.

I think you probably can name it as tasksFromFile.

Good Job! Clear explanation of the symbol use for different tasks.

Should this import sentence be put above importing our own class？

Same as above.

Same advice as above for coding standard regarding to import.

Should we use explicit import instead of wild card import?

Is it better if we leave a line between different importing group?

Maybe is it better to add JavaDoc here to help decribe Searchable?

Perhaps you can change "existing" to "exists" to make it sound more boolean!

I think you can change the name of "at" to make it more descriptive! Maybe something like "eventDate".

I think you can change this method name to a verb, maybe something like "incrementTaskCount".

Same as before, maybe something like "reduceTaskCount".

You may have missed out a space before the variable name here.

Shouldn't this variable be capitalized as it is a constant? Similar problem found below.

Not sure if the verb form here is not appropriate, similar problems encountered with other comments

Shouldn't this be phrased like other methods where you start with a verb? Probably something on the lines of 'Constructs...'

Good usage of @Override which explicitly labels methods that overrides parent classes.

Shouldn't the header comments be included as this is a public method? Similar problems encountered for other methods in this class.

Hi Wei Xue! You can try to delete the output lines before committing, just in case it affects with testing.

Perhaps you can list out the exceptions that you will be using in this file.

According to the modules' coding standards, case and default should be at the same indentation as the switch statement.

Hi Douglas! You might want to edit the javadocs for this since it only returns false.

Since it's not a DBotException, you can remove "throws DBotException".

Perharps you can use a better naming for this variable?

Perharps you can introduce this line into the UI class?

Perharps you can use a clearer and more intuitive variable name here! Other than that your coding standards looks good!

Careful of the spacing over here! Depending on how your code runs, the spaces might affect the print of output!

For debugging I see!

Similiar mistake as me! We can change the variable name 'by' to be more intuitive

String array name can be more descriptive 😄

Since there are many if else statements, I think using switch would make the code look cleaner 😃

As per coding standard, boolean variable name would be a name to point out that the variable is boolean. E.g ("isDone") would be good 😄

I think the @author tag can be placed at the top of the class along with the Javadoc to provide more consistency in the JavaDoc format! Other than that, your codes' naming and format is really clean and I enjoyed reading it!

The result of createNewFile() is unused, maybe try to put it with another if else statement

Maybe just isBye() is enough

Consider using a clearer name, ie deadline, date

Consider trying out switch case statements

Consider using a clearer name

Should there be a whitespace between Java reserved words and curly brackets?

Maybe have consistent line breaks within the body of a method? (Compare this with lines 52~55)

Maybe have consistent line breaks within the body of a method? (Compare this with lines 28~31)

Are curly braces required after case statement? I understand if it's a personal preference.

Smart way to determine whether 'task' should be plural or not

Maybe more methods could be added here to reduce the nested if brackets?

Could remove one blank line here

I think there should be an empty line before this statement

Maybe this boolean variable could be isDone to improve clarity?

I think maybe there should be an empty line before this statement?

Perhaps splitting the separator string into 2 lines using a "+" would be neater according to coding style guidelines?

Perhaps it'd be more suitable if this String is named something eg. description instead of deadLine?

    String deadLine;

    String description;

I like how your localDate and localTime are in one line. Clean 😃

Perhaps you can shorten the line to make your code more readable according to coding guidelines?

LOL!

Maybe you could consider naming the boolean to isDone such that it is more understandable?

Perhaps you can consider using switch cases to minimize the need for if-else statements as it would improve the readability of your code?

This function is named "finishATask" while another one is "addTask", maybe you could pick either way to name the function and standardize it across your functions so as to keep it more consistent (i.e. either name it (verb)ATask or (verb)Task)?

Haha cute name, but maybe you could consider changing it to something more understandable like taskList?

I think under the Java coding standards the convention for a for loop is for (initialization; condition; update) (although I also use enhanced loops), you could consider changing it?

Although this does follow the naming to sound like a boolean, but just a minor issue that isExit as phrase does not seem right to me

I do agree with yutingzou on this

Could perhaps use a actual word to name the variable spl, as it is being used throughout the parse code

also for the checkSplLength below

I believe the starting letter should be lower caps according to coding standard

Perhaps this should be 'SERIAL_VERSION_UID' as it is a constant?

Maybe boolean could be named isCompleted instead to sound more like a boolean?

I like that you followed the switch coding format! I sometimes forget to do this.

Perhaps this variable could be named 'dateBy' or 'dueDate' for more clarity?

Hmm, switch case does not seem to follow the coding standard. The 'case' should be indented at the same level as 'switch'! 😃

Perhaps the name of this LocalDate variable could also be replaced by a clearer one?

Should there be an empty line between the description and parameter section?

Maybe the else statement on line 41 should be added to line 40 instead?

I like how you separated the string constants into a class of its own!

Perhaps there should be an empty line between the description and parameter section? I have noticed the same issue in several other places too

Well done, naming is good and coding standard is strictly followed!

Should you use Pascalcase to name the enum? ie TaskState

Generally the code is very clean, good job!

Should you use Camel case to name this method?

Should you delete the space between two import statements?

perhaps there should be spacings between the variables and the value?

like distributionBase = GRADLE_USER_HOME and the sorts?

Does this main function do anything? If not maybe you can delete it to make your code look neater?

I like the way you separated the code over here! It makes it easier to read the long sentence.

perhaps there could be a spacing here? like try { instead of try{

but I'm pretty sure this is just a careless mistake so don't worry too much about it!

I like the way you abstracted these!

Would it be better to set default value false for Task so that the subclasses do not require setting it in their respective constructors?

Is the syntax "this" necessary for calling the echo method?

I like your use of the StringBuilder! Beats concatenation of Strings when output becomes larger. Did you choose to write the entire output at once over line by line for performance reasons?

Is the type inference with var necessary since it is assigned to Dialog?

do you think maybe switch statements could be used to make the code look more concise?

Maybe some java docs will be useful to make the user understand the code better!

I think it is great that you implemented Duke Exceptions early!

I think it's good that you kept the line of print statement short:>

Same comment as before.

Also the same as before

Magic number, could use an enum or static variable in the class

Would it be better if the input were to be split into two types? One that adds tasks, and the other type does not. It would reduce the total line count of the function, making it easier to read.

Hi, i think maybe you should refactor the code to improve readability as the method is very long.

The recommended guideline would be not to go beyond 30 LOC (lines of code)

Hi , maybe you should create a named constant for these responses for better readability as recommended in Section 5.4e

Hi, maybe you should throw an exception here? Leaving it blank may result in future bugs.

Hi, very nice use of SLAP to keep in within 30 LOC