-
Notifications
You must be signed in to change notification settings - Fork 78
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
[YFshadaow] ip #80
base: master
Are you sure you want to change the base?
[YFshadaow] ip #80
Conversation
@@ -5,11 +5,12 @@ | |||
public class CommandHandler { | |||
|
|||
private final XiaoAiBot bot; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the naming of the robot!
case "todo": { | ||
Todo todo; | ||
try { | ||
todo = Todo.parseTodo(args); | ||
} catch (Exception e) { | ||
bot.sendMessage("Error parsing task"); | ||
e.printStackTrace(); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of try-catch function!
case "deadline": { | ||
Deadline deadline; | ||
try { | ||
deadline = Deadline.parseDeadline(args); | ||
} catch (Exception e) { | ||
bot.sendMessage("Error parsing task"); | ||
e.printStackTrace(); | ||
break; | ||
} | ||
bot.getTasks().add(deadline); | ||
bot.sendMessageWithoutSplit("Got it. I've added this task:"); | ||
bot.sendMessageWithoutSplit(deadline.toStringWithIsDone()); | ||
bot.sendMessage("Now you have " + bot.getTasks().size() + " tasks in the list."); | ||
break; | ||
} | ||
case "event": { | ||
Event event; | ||
try { | ||
event = Event.parseEvent(args); | ||
} catch (Exception e) { | ||
bot.sendMessage("Error parsing task"); | ||
e.printStackTrace(); | ||
break; | ||
} | ||
bot.getTasks().add(event); | ||
bot.sendMessageWithoutSplit("Got it. I've added this task:"); | ||
bot.sendMessageWithoutSplit(event.toStringWithIsDone()); | ||
bot.sendMessage("Now you have " + bot.getTasks().size() + " tasks in the list."); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of switch-case statements!
|
||
private String by; | ||
|
||
public Deadline(String name, String by) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good use of constructor!
this.bot = bot; | ||
} | ||
|
||
public void handleCommand(String command) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very clear naming of the methods. Makes it readable and easy to understand.
case "todo": { | ||
Todo todo; | ||
try { | ||
todo = Todo.parseTodo(args); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to handle the exception more gracefully rather than printing the stack trace and breaking the switch block? For example, could we log the error and send a user-friendly message instead?
bot.sendMessage("No command detected!"); | ||
return; | ||
} | ||
String[] args = new String[splitCommand.length - 1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could it be clearer to use Arrays.copyOfRange() here? It might make the code more readable.
break; | ||
} | ||
bot.sendMessageWithoutSplit("OK, I've marked this task as not done yet:"); | ||
bot.sendMessage(bot.getTasks().get(index - 1).toStringWithIsDone()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more efficient to store the result of bot.getTasks().get(index - 1) in a variable before sending it as a message? This way, we wouldn't need to retrieve it twice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usually I do that for variables that are used three times or more. For variables that are used twice I might just leave it as that.
@@ -0,0 +1,132 @@ | |||
package cn.yfshadaow.cs2113.ip; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Zhengdao! Hope your health got better over the holidays!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good adherence with coding standards and use of exceptions.
public String getBy() { | ||
return by; | ||
} | ||
|
||
public void setBy(String by) { | ||
this.by = by; | ||
} | ||
|
||
private String by; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easier to read f you declare first and use later
|
||
private boolean shouldQuit = false; | ||
|
||
private void initialize() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
dont put unfinished code in a pr
Add find command: Allow user search for tasks using keywords
Add javadoc
No description provided.