-
Notifications
You must be signed in to change notification settings - Fork 364
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
[sarthak181] iP #225
base: master
Are you sure you want to change the base?
[sarthak181] iP #225
Conversation
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 code overall besides a few changes that should be made to follow our module's coding standard. Do remember to add descriptive header comments (eg. javadocs) to your public classes and methods. Keep up the good work! 👍
src/main/java/Duke.java
Outdated
switch (getSwitch(str)) { | ||
case 0: | ||
int n; | ||
Task t; | ||
String num; | ||
num = str.substring(5); | ||
n = Integer.parseInt(num) - 1; | ||
t = list.get(n); | ||
t.markAsDone(); | ||
System.out.println("Nice! I've marked this task as done:\n" | ||
+ " [" + t.getStatusIcon() + "] " + t.description); | ||
break; | ||
case 1: | ||
int n_1; | ||
Task t_1; | ||
String num_1; | ||
num_1 = str.substring(7, 9); | ||
n_1 = Integer.parseInt(num_1) - 1; | ||
t_1 = list.get(n_1); | ||
t_1.unMark(); | ||
System.out.println("OK, I've marked this task as not done yet:\n" | ||
+ " [" + t_1.getStatusIcon() + "] " + t_1.description); | ||
break; | ||
case 2: | ||
Task a; | ||
try { | ||
a = new Todo(str.substring(5)); | ||
} catch (DukeException e) { | ||
System.out.println(e.getMessage()); | ||
break; | ||
} | ||
list.add(a); | ||
System.out.println("Got it. I've added this task:\n " | ||
+ a.toString() + "\nNow you have " + list.size() | ||
+ " tasks in the list."); | ||
break; | ||
case 3: | ||
int ind = str.indexOf("/by"); | ||
Task b = null; | ||
try { | ||
b = new Deadline(str.substring(9, ind), str.substring(ind+3)); | ||
} catch (DukeException e) { | ||
System.out.println(e.getMessage()); | ||
break; | ||
} | ||
list.add(b); | ||
System.out.println("Got it. I've added this task:\n " | ||
+ b.toString() + "\nNow you have " + list.size() | ||
+ " tasks in the list."); | ||
break; | ||
case 4: | ||
int index_1 = str.indexOf("/from"); | ||
int index_2 = str.indexOf("/to"); | ||
Task c = null; | ||
try { | ||
c = new Event(str.substring(6, index_1), str.substring(index_1+5, index_2), | ||
str.substring(index_2 + 3)); | ||
} catch (DukeException e) { | ||
System.out.println(e.getMessage()); | ||
break; | ||
} | ||
list.add(c); | ||
System.out.println("Got it. I've added this task:\n " | ||
+ c.toString() + "\nNow you have " + list.size() | ||
+ " tasks in the list."); | ||
break; | ||
case 5: | ||
System.out.println("☹ OOPS!!! I'm sorry, but I don't know what that means :-("); | ||
|
||
} |
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.
cases in switch statements should not be indented. eg
switch (variable) {
case 0:
return 0;
case 1:
return 1;
}
src/main/java/Duke.java
Outdated
if (str.startsWith("mark ")) return 0; | ||
if (str.startsWith("unmark ")) return 1; | ||
if (str.startsWith("todo ")) return 2; | ||
if (str.startsWith("deadline ")) return 3; | ||
if (str.startsWith("event ")) return 4; |
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.
Use K&R style brackets. eg
if (condition) {
return 0;
}
I suggest just using the string directly in the switch statement but this is entirely up to you.
src/main/java/Duke.java
Outdated
@@ -1,3 +1,5 @@ | |||
import java.util.*; | |||
|
|||
public class Duke { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Classes should be placed in a package. You can just place all your current class in a duke package minimally
src/main/java/Event.java
Outdated
protected String from; | ||
protected String to; | ||
|
||
public Event(String description, String from, String to) throws DukeException{ |
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.
'DukeException{'. Missed out on a space between n and {.
src/main/java/Duke.java
Outdated
@@ -1,3 +1,5 @@ | |||
import java.util.*; |
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.
Wildcard imports should not be used. Imported classes should be listed explicitly
# Conflicts: # src/main/java/duke/TaskList.java
Merge Added Assertions
Changed the way that the code responds when an exception is caught. Changed it so that when an exception is caught, the Duke bot actually tells the user that there is an error.
This help command is simply the user typing help in all lowercase and hitting enter. The commands aren't too easy to remember, so this help command will make it easier to use the app.
No description provided.