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

Add more feature to the bot #199

Open
wants to merge 44 commits into
base: master
Choose a base branch
from

Conversation

drake25122000
Copy link

Add some features and converting the project into more OOP based.
Also add javadocs and follow the coding standard

}

@Override
public boolean equals(Object o) {

Choose a reason for hiding this comment

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

Maybe it is better to name the object with another better name that describes the object better?

/**
* Check if the current command is an exit command
**/
public abstract boolean isExit();

Choose a reason for hiding this comment

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

Perhaps it is better to set isExit as a boolean attribute?

public String toString() {
return isDone
? "[D][✓] " + this.getName() + " (by: "
+ this.deadline.format(DateTimeFormatter.ofPattern("MMM d yyyy HHmm")) + ")"

Choose a reason for hiding this comment

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

I think it will be better to indent four spaces before the '+' as this is break of a statement.

}
}

private static int decideIndexDelete(String word) throws MissingSpecifiedDeleteError {

Choose a reason for hiding this comment

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

Maybe it is better to name the method as decideDeleteIndex as the index is the most concerned word.

Copy link

@patricktan6 patricktan6 left a comment

Choose a reason for hiding this comment

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

Overall the code quality is very good. LGTM.

Copy link

@kkangs0226 kkangs0226 left a comment

Choose a reason for hiding this comment

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

Keep up the good work!!:) There weren't many changes to be made

/**
* Initializes AddCommand class
*
* @param command

Choose a reason for hiding this comment

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

I think the recommended JavaDoc for methods with parameters is that you describe the parameter in a one-liner, so maybe "@param command command of user" would be better!

/**
* Initializes DeleteCommand
*
* @param command

Choose a reason for hiding this comment

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

As above, here you could include the description of your parameter

* Load all the tasks from the save file to ArrayList<Task>
**/
public ArrayList<Task> load() throws DukeException {
ArrayList<Task> temp = new ArrayList<>();

Choose a reason for hiding this comment

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

I think according to coding standards they recommended that we put plural form for collections such as lists, so here maybe 'tasks' would be a better variable name!

}
} else {
try {
String currname = name.substring(6);

Choose a reason for hiding this comment

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

I think according to coding standards it should be currName and not currname!

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

3 participants