Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Jeremy Tan] iP #326

Closed
wants to merge 53 commits into from
Closed

[Jeremy Tan] iP #326

wants to merge 53 commits into from

Conversation

koonweee
Copy link

No description provided.

}
}

class FindCommand extends Command {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldnt different classes be put into different files ?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, for now the code is manageable as they are not too much, but if the project expands, it may become hard to navigate.

* Formats given String and prints in the UI
* @param printStr String to be formatted and printed
*/
public void printf(String printStr) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this function name should be more expressive

@daongochieu2810
Copy link

Great job with the current iP progress! The code is neat and most classes have specific methods with 1 clear purpose. However there are several points that could be improved. The testing seems to be a bit sparse, you might want to add more tests as you have quite a number of methods for each class.
Good job overall!

class DeleteCommand extends Command {
private int deleteTask;

DeleteCommand(int task) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can consider renaming the task parameter as taskIndex to make it clearer for users

* A delete command to delete a given task number
*/
class DeleteCommand extends Command {
private int deleteTask;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be named better, maybe taskIndexDelete, etc..

Copy link

@ChenXJ98 ChenXJ98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall really great! I love that your code is very neat and easy to understand. Just some nit picks actually. :D

class TodoCommand extends Command {
private String task;

TodoCommand(String toParse) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i agree that it will be better to split the various classes into different files

Comment on lines 50 to 57
public static DialogBox getUserDialog(String text, Image img) {
return new DialogBox(text, img);
}

public static DialogBox getDukeDialog(String text, Image img) {
var db = new DialogBox(text, img);
db.flip();
return db;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if Javadocs is needed since the code is provided.

Comment on lines 69 to 72
/**
* You should have your own function to generate a response to user input.
* Replace this stub with your completed method.
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a placeholder for now?

return Arrays.asList(rtnSplit).stream().collect(Collectors.joining("\n"));
}

public String find(String key) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better if function is renamed to finding a specific thing instead of just find.

String[] split = line.split("%d%");
if (split.length > 3) {
taskList.add(Task.createTask(split[0], split[1], split[2], split[3]));
} else {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can add an additional else statement to capture everything else. For easier debugging.

@koonweee koonweee closed this Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants