-
Notifications
You must be signed in to change notification settings - Fork 252
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
[AY1920S1-CS2113T-F09-4] Ducats #29
base: master
Are you sure you want to change the base?
[AY1920S1-CS2113T-F09-4] Ducats #29
Conversation
looks good |
[CS2113T-T14-2]-CEGLincoln-B-DoWithinPeriodTasks
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.
Should use enum instead of numbers and strings.
Should remove necessary classes and comments.
There are a lot of magic strings in code, should define as variables.
for (Task temp : taskTempList) { | ||
LocalDateTime startDateCheck = checkTask.getStartDate(); | ||
LocalDateTime endDateCheck = checkTask.getEndDate(); | ||
if (temp.getType().equals("E")) { |
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.
no magic string/character
src/main/java/duke/DatePattern.java
Outdated
public ParseTime.TimePatternType get_type() { | ||
return type; | ||
} | ||
|
||
public String get_pattern() { | ||
return pattern; | ||
} |
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 camel case
src/main/java/duke/Duke.java
Outdated
import java.util.Timer; | ||
import java.util.TimerTask; | ||
|
||
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.
should be your own product
src/main/java/duke/Duke.java
Outdated
// try { | ||
// storage.loadList(tasks); | ||
// } catch (DukeException e) { | ||
// System.out.println(ui.showError(e)); | ||
// tasks = new TaskList(); | ||
// } |
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.
delete
src/main/java/duke/Duke.java
Outdated
boolean isExit = false; | ||
TimerTask repeatedTask = new TimerTask() { | ||
public void run() { | ||
Command c = new RemindCommand(); |
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.
should give meaningful variable name
if (input.trim().equals("todo") || input.trim().equals("event") | ||
|| input.trim().equals("deadline") || input.trim().equals("doafter") | ||
|| input.trim().equals("new") || input.trim().equals("view") | ||
|| input.trim().equals("addbar")) { | ||
|
||
message = "OOPS!!! The description of " | ||
+ word | ||
+ input.trim() | ||
+ " task cannot be empty."; | ||
} else if (!type.equals("other")) { | ||
switch (type) { | ||
case "todo": { | ||
message = "OOPS!!! I'm sorry, but I don't know what that means :-("; | ||
break; | ||
} | ||
case "event": { | ||
if (!input.contains("/at")) { | ||
message = "OOPS!!! duke.tasks.Event is missing a location."; | ||
} | ||
break; | ||
} | ||
case "deadline": { | ||
if (!input.contains("/by")) { | ||
message = "OOPS!!! duke.tasks.Deadline is missing a deadline."; | ||
} | ||
break; | ||
} | ||
case "doafter": { | ||
if (!input.contains("/after")) { | ||
message = "OOPS!!! duke.tasks.DoAfter is missing a task it is supposed to be done after."; | ||
} else { | ||
message = "Please enter the task number of the task that the DoAfter should be after."; | ||
} | ||
break; | ||
} | ||
case "new": { | ||
message = "OOPS!!! duke.components.Song cannot be created due to invalid input format."; | ||
break; | ||
} | ||
case "view": { | ||
message = "OOPS!!! I don't know that song. Please be more specific in the song name."; | ||
break; | ||
} | ||
case "addbar": { | ||
message = "OOPS!!! New bar cannot be added due to invalid input."; | ||
break; | ||
} | ||
case "io": { | ||
message = "OOPS!!! An IO exception has occurred."; | ||
break; | ||
} | ||
case "no_index": { | ||
message = "The index does not exist!"; | ||
break; | ||
} | ||
case "empty": { | ||
message = "List is empty! Please enter a valid command."; | ||
break; | ||
} | ||
case "conflict": { | ||
message = "There is a conflict between this event and another event!"; | ||
break; | ||
} | ||
case "index": { | ||
message = "Invalid index! Please try again."; | ||
break; | ||
} | ||
case "between": { | ||
message = "Invalid input for a between task. " | ||
+ "Please follow this format: between <task_description> /between <start> and <end>"; | ||
break; | ||
} | ||
case "recur": { | ||
message = "Invalid input for a recurring task. Please follow this format:" | ||
+ " recur <frequency> <description> /on <date> /at <time>\n"; | ||
message += "<frequency> could only be one of: daily, weekly, monthly or yearly\n"; | ||
message += "<date> has to follow the specific format of: dd/mm/yy\n"; | ||
message += "/at <time> is optional."; | ||
break; | ||
} | ||
case "group": { | ||
message = "OOPS!!! These groups cannot be grouped due to invalid input."; | ||
break; | ||
} | ||
case "copy": { | ||
message = "OOPS!!! Invalid input for copy command."; | ||
break; | ||
} | ||
default: { | ||
message = "OOPS!!! I'm sorry, but I don't know what that means :-("; | ||
} | ||
} | ||
} else { | ||
message = "OOPS!!! I'm sorry, but I don't know what that means :-("; | ||
} | ||
return Ui.wrap(message); | ||
// wrap is called from Ui in order to standardize the formatting of the output | ||
} |
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.
should use enum
src/main/java/duke/Parser.java
Outdated
* @throws DukeException in the case of parsing errors | ||
*/ | ||
static Command parse(String message) throws DukeException { | ||
switch (message.split(" ")[0]) { |
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 enum
src/main/java/duke/Ui.java
Outdated
Integer pasteStartNum, | ||
int mode) { | ||
String result; | ||
if (mode == 0) { |
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 enum
@@ -0,0 +1,25 @@ | |||
package duke.commands; |
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.
remove necessary class
src/main/java/duke/tasks/Task.java
Outdated
@@ -0,0 +1,67 @@ | |||
package duke.tasks; |
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.
remove duke classes
Update to fix parsing error
Fix JAR storage issue
Refactor code to have better Command syntax consistency
fixed bugs
fixed bug for autosuggest
PR W12 Tuesday 1011
Create new exception to prevent same song names
PR W12 Tuesday 1220
* Finish PPP entirely and Finish Appendix E for DG * Update to v1.4
* 'master' of https://github.com/SalonetheGreat/main: Update to v1.4 (#251) Update UG and DG (#250) updated ug 4 updated ug 4
* 'master' of https://github.com/SalonetheGreat/main: ppp done
* Add ascii song for UG * Fix a bug in UG
Add ascii song for UG (#253)
Fix image issues with adocs
PR W13 Tue 0023
* Finish PPP entirely and Finish Appendix E for DG * Update to v1.4 * Final update
Add overlay_group_group_1_2.png
* 'master' of https://github.com/SalonetheGreat/main: Update UG (#255)
Add Command Summary
@jwyf
@SalonetheGreat
@rishi12438
@Samuel787