-
Notifications
You must be signed in to change notification settings - Fork 438
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
[Nguyen Hoang Hai Minh] iP #445
base: master
Are you sure you want to change the base?
Changes from 20 commits
3b19ba1
cb0dcf9
ff00562
299005e
052071c
d53eb38
c9c67ac
7345f13
61ace1f
828bf0b
87a8790
eff84ea
716d3c3
a9a8581
9e2f0f5
12fe9af
758241f
367da21
d47603e
3f6b540
53a20a7
e5257ac
f71b3c9
d5b4965
af6a803
ca6ec8c
7428cf4
8e6c851
d3b8993
396f602
9372042
8803587
25a54ce
23a3044
9e14f33
d2cc0a8
208bff7
8f99e1d
44ec70b
e1a5959
a5219a1
2b4f16d
29bd1ae
e393d3d
692c58e
d4b0d9d
6e5ad76
e8b9524
00f4ff0
426b401
f9481a7
0cef394
23525e3
407b183
462aaa7
28b3deb
09a96b7
1f73d1d
2e39786
befc6ad
c2d3a3d
c24f512
b28e166
779eb86
e88c219
601725b
012b4f6
ce880a7
1ed4376
08f971a
18e4cee
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
10 | ||
T|read book|false | ||
T|read book|true | ||
T|return book /by June 6|false | ||
D|return book|June 6|true | ||
E|dinner|6 30|false | ||
T|cak|false | ||
T|sleep|false | ||
D|sleep|6|false | ||
D|sleep|Oct 15 2019|false | ||
D|sle|23|false |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
//package duke; | ||
/** | ||
* Deadline <- Task | ||
*/ | ||
public class Deadline extends Task { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like how all the class names in your code and clear, concise and adhered to the Java coding standards and naming conventions. |
||
String description; | ||
String deadline; | ||
boolean isDone; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these class variables be declared with a non-public access modifier to adhere to encapsulation principles? Perhaps you could consider using protected or private? |
||
|
||
/** | ||
* Init Deadline | ||
*/ | ||
public Deadline(String description, String deadline) { | ||
super(description); | ||
this.deadline = deadline; | ||
} | ||
|
||
/** | ||
* Status in format [D][x] return book (by: 6 June) | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could consider following the guidelines for Java coding standards when writing JavaDoc comments? I think you are missing a 'return parameter' for the JavaDoc comment, or perhaps is there any reason why you wrote the JavaDoc comments in the way you did? I observed the same issue in several other methods as well. |
||
@Override | ||
public String getStatus() { | ||
return "[D]" + super.getStatus() + " (by: " + deadline + ")"; | ||
} | ||
|
||
/** | ||
* type of Task -> D for deadline | ||
*/ | ||
@Override | ||
public String getType() { | ||
return "D"; | ||
} | ||
|
||
/** | ||
* description to write to data file | ||
*/ | ||
@Override | ||
public String getDescription() { | ||
return super.getDescription() + "|" + this.deadline; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,10 +1,152 @@ | ||
//package duke; | ||
import java.io.IOException; | ||
import java.util.*; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could list out all the imported classes explicitly? I noticed the same issue in several other classes as well. |
||
import java.io.File; | ||
import java.io.FileWriter; | ||
import java.io.FileNotFoundException; | ||
import java.time.LocalDate; | ||
import java.time.format.DateTimeFormatter; | ||
/** | ||
* Duke the best chatbot hehe | ||
*/ | ||
public class Duke { | ||
|
||
private Storage storage; | ||
private TaskList tasks; | ||
private Ui ui; | ||
private Parser parser; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could consider adding JavaDoc comments for class members as well? |
||
|
||
/** | ||
* Init Duke | ||
*/ | ||
public Duke() { | ||
ui = new Ui(); | ||
storage = new Storage(); | ||
tasks = new TaskList(storage.readData()); | ||
parser = new Parser(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this way of initialising the variables as its very concise. However, perhaps you could consider using "this" to refer to the instance variables? I feel like it would be a more explicit representation of the initialisation in the constructor. |
||
} | ||
|
||
/** | ||
* Main | ||
*/ | ||
public static void main(String[] args) { | ||
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
new Duke().run(); | ||
} | ||
|
||
/** | ||
* Take in command | ||
*/ | ||
public void run() { | ||
ui.sayHi(); | ||
Scanner myScanner = new Scanner(System.in); | ||
while(true) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could add a whitespace after 'while'? The guidelines on Java coding standards states that Java reserved words should be followed by a white space. I noticed this issue in other methods containing while, if and if-else blocks as well. |
||
String cmd = myScanner.nextLine(); | ||
if(cmd.equals("bye")) { | ||
ui.sayBye(); | ||
break; | ||
} | ||
else if(cmd.equals("list")) { | ||
System.out.println("Here are the tasks in your list:"); | ||
for(int i = 1; i <= tasks.getSize(); ++i) { | ||
System.out.println(i + "." + tasks.get(i - 1).getStatus()); | ||
} | ||
} | ||
else if(cmd.length() >= 4 && cmd.substring(0, 4).equals("done")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could use comments to further elaborate on your "else if" statements' condition? I feel like it would definitely be clearer and will increase readability for readers! |
||
int c = 0; | ||
for(int i = 5; i < cmd.length(); ++i) { | ||
c = c * 10 + cmd.charAt(i) - '0'; | ||
} | ||
System.out.println("Nice! I've marked this task as done:"); | ||
tasks.get(c - 1).done(); | ||
System.out.println(tasks.get(c - 1).getStatus()); | ||
} | ||
else if(cmd.length() >= 4 && cmd.substring(0, 4).equals("todo")) { | ||
try { | ||
checkCmd(cmd, 4, "☹ OOPS!!! The description of a todo cannot be empty."); | ||
String getName = cmd.substring(5); | ||
Todo tmp = new Todo(getName); | ||
System.out.println("Got it. I've added this task: "); | ||
System.out.println(" " + tmp.getStatus()); | ||
tasks.add(tmp); | ||
System.out.println("Now you have " + tasks.getSize() + " tasks in the list."); | ||
} | ||
catch (DukeException ex) { | ||
System.out.println(ex.getMessage()); | ||
} | ||
} | ||
else if(cmd.length() >= 8 &&cmd.substring(0, 8).equals("deadline")) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should there be an extra whitespace after the '&&' operator? I noticed the same issue in other areas as well. |
||
try { | ||
checkCmd(cmd, 8, "☹ OOPS!!! The description of a deadline cannot be empty."); | ||
String getName = parser.getNameBy(cmd); | ||
String getDeadline = parser.getDeadlineBy(cmd); | ||
getDeadline = formatDate(getDeadline); | ||
Deadline tmp = new Deadline(getName, getDeadline); | ||
System.out.println("Got it. I've added this task: "); | ||
System.out.println(" " + tmp.getStatus()); | ||
tasks.add(tmp); | ||
System.out.println("Now you have " + tasks.getSize() + " tasks in the list."); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate how clean your code is in the try blocks! But maybe you could include more spacings and comments between different segments of the code so that it is easier to capture the main gist of your code? |
||
} | ||
catch (DukeException ex) { | ||
System.out.println(ex.getMessage()); | ||
} | ||
} | ||
else if(cmd.length() >= 5 &&cmd.substring(0, 5).equals("event")) { | ||
try { | ||
checkCmd(cmd, 5, "☹ OOPS!!! The description of a event cannot be empty."); | ||
String getName = parser.getNameAt(cmd); | ||
String getTime = parser.getDeadlineAt(cmd); | ||
getTime = formatDate(getTime); | ||
Event tmp = new Event(getName, getTime); | ||
System.out.println("Got it. I've added this task: "); | ||
System.out.println(" " + tmp.getStatus()); | ||
tasks.add(tmp); | ||
System.out.println("Now you have " + tasks.getSize() + " tasks in the list."); | ||
// | ||
} | ||
catch (DukeException ex) { | ||
System.out.println(ex.getMessage()); | ||
} | ||
} | ||
else if(cmd.length() >= 6 && cmd.substring(0, 6).equals("delete")){ | ||
int c = 0; | ||
for(int i = 7; i < cmd.length(); ++i) { | ||
c = c * 10 + cmd.charAt(i) - '0'; | ||
} | ||
System.out.println("Noted. I've removed this task: "); | ||
System.out.println(tasks.get(c - 1).getStatus()); | ||
tasks.remove(c - 1); | ||
System.out.println("Now you have " + tasks.getSize() + " tasks in the list."); | ||
} | ||
else if(cmd.length() >= 4 && cmd.substring(0, 4).equals("find")) { | ||
String tmp = cmd.substring(5); | ||
System.out.println("Here are the matching tasks in your list:"); | ||
for(int i = 1; i <= tasks.getSize(); ++i) if(tasks.get(i - 1).description.contains(tmp)){ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you could use Egyptian style brackets for the 'for' loop? Also, maybe you can consider writing the 'if' block on the next line below the 'for' statement? I think this can make the code look cleaner and enhances readability. |
||
System.out.println(i + "." + tasks.get(i - 1).getStatus()); | ||
} | ||
} | ||
else 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 commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps the conditional else statement body should be put on a separate line, i.e line 128? Also, maybe you could wrap the body of the else statement in curly braces to enhance readability? |
||
storage.updateDataFile(tasks.getArrayList()); | ||
} | ||
} | ||
|
||
/** | ||
* check if date is yyyy-mm-dd, then format to MMM d yyyy | ||
*/ | ||
public static String formatDate(String str) { | ||
LocalDate d; | ||
try { | ||
d = LocalDate.parse(str); | ||
} catch (Exception e) { | ||
return str; | ||
} | ||
return d.format(DateTimeFormatter.ofPattern("MMM d yyyy")); | ||
} | ||
|
||
/** | ||
* check if the command is valid | ||
*/ | ||
public static void checkCmd(String cmd, int len, String Ex) throws DukeException { | ||
if(cmd.length() == len) throw new DukeException(Ex); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
//package duke; | ||
/** | ||
* handle error when using duke | ||
*/ | ||
public class DukeException extends Exception { | ||
public DukeException(String message) { | ||
super(message); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
//package duke; | ||
/** | ||
* Event <- Task | ||
*/ | ||
public class Event extends Task { | ||
String description; | ||
String time; | ||
boolean isDone; | ||
|
||
/** | ||
* Init Event | ||
*/ | ||
public Event(String description, String time) { | ||
super(description); | ||
this.time = time; | ||
} | ||
|
||
/** | ||
* Status in format [E][x] return book (at: 6 June) | ||
*/ | ||
@Override | ||
public String getStatus() { | ||
return "[E]" + super.getStatus() + " (at: " + time + ")"; | ||
} | ||
|
||
|
||
/** | ||
* type of Task -> E for Event | ||
*/ | ||
@Override | ||
public String getType() { | ||
return "E"; | ||
} | ||
|
||
/** | ||
* description to write to data file | ||
*/ | ||
@Override | ||
public String getDescription() { | ||
return super.getDescription() + "|" + this.time; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Manifest-Version: 1.0 | ||
Main-Class: Duke | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,62 @@ | ||
//package duke; | ||
/** | ||
* translate command -> name and time | ||
*/ | ||
public class Parser { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like how your Parser class separates different functions in a very intuitive manner to readers. However, perhaps you could provide a more detailed naming to the functions? This is so that the functionality of your functions can be more easily inferred by its name. |
||
|
||
public Parser() { | ||
|
||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you can leave this constructor out since Java creates a default constructor if no constructor was declared. However, I do not think this is a serious issue. I noticed a similar issue in the Storage class below as well. |
||
|
||
/** | ||
* get name | ||
* by command | ||
*/ | ||
public String getNameBy(String cmd) { | ||
for (int i = 0; i < cmd.length(); ++i) { | ||
if (cmd.charAt(i) == '/' && cmd.charAt(i + 1) == 'b' && cmd.charAt(i + 2) == 'y') { | ||
return cmd.substring(9, i - 1); | ||
} | ||
} | ||
return ""; | ||
} | ||
|
||
/** | ||
* get deadline | ||
* by command | ||
*/ | ||
public String getDeadlineBy(String cmd) { | ||
for (int i = 0; i < cmd.length(); ++i) { | ||
if (cmd.charAt(i) == '/' && cmd.charAt(i + 1) == 'b' && cmd.charAt(i + 2) == 'y') { | ||
return cmd.substring(i + 4); | ||
} | ||
} | ||
return ""; | ||
} | ||
|
||
/** | ||
* getname | ||
* at command | ||
*/ | ||
public String getNameAt(String cmd) { | ||
for (int i = 0; i < cmd.length(); ++i) { | ||
if (cmd.charAt(i) == '/' && cmd.charAt(i + 1) == 'a' && cmd.charAt(i + 2) == 't') { | ||
return cmd.substring(6, i - 1); | ||
} | ||
} | ||
return ""; | ||
} | ||
|
||
/** | ||
* get time | ||
* at command | ||
*/ | ||
public String getDeadlineAt(String cmd) { | ||
for (int i = 0; i < cmd.length(); ++i) { | ||
if (cmd.charAt(i) == '/' && cmd.charAt(i + 1) == 'a' && cmd.charAt(i + 2) == 't') { | ||
return cmd.substring(i + 4); | ||
} | ||
} | ||
return ""; | ||
} | ||
} |
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 there a better way to describe the Deadline class in the Javadoc comment? I understand what you are trying to convey but perhaps it would be better to elaborate on it! I personally prefer this representation as its very concise however it might not be intuitive enough for other readers.