-
Notifications
You must be signed in to change notification settings - Fork 270
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
[Adrian Ong] iP #302
base: master
Are you sure you want to change the base?
[Adrian Ong] iP #302
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.
Overall, an elegant and tidy set of code! Can distinguish the flow of the programme and it would be easy to take over as a public repo if ever you are going to publish this project 😄
Just some tiny pointers about the indentation and also whitespace! Otherwise, good job 👍🏼
src/main/java/Duke.java
Outdated
for(int i = 0; i < masterList.size(); i++) { | ||
Task curr = masterList.get(i); | ||
bw.write((i + 1) + "." + curr.toString()); | ||
bw.newLine(); | ||
} | ||
bw.flush(); |
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.
A very intuitive set of code!
I was wondering if there's also a need for whitespace after the "for" word?
Nonetheless, neat work :)
src/main/java/DukeException.java
Outdated
public DukeException(String message) { | ||
super(message); | ||
} |
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.
Awesome that this class is kept simple!
However, maybe there should be some comments to document on what kind of exceptions you are expecting? 😄
src/main/java/Duke.java
Outdated
System.out.println("Hello! I'm Duke\nWhat can I do for you?"); | ||
String input; // to store raw input command | ||
String[] inputArr; // to store split input command | ||
boolean ifBye = false; |
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.
Intuitive boolean to check if the program should exit or not!
However, maybe:
boolean ifBye = false; | |
boolean shouldAbort = false; |
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.
LGTM after sorting out a few nits. Overall, the code adhered well to Java coding conventions. Spacing of comments and code appears to be irregular at times, so do keep a look out for them. You can also consider adding file types of ".class" to gitignore so as to not push them to remote.
Good job!
src/main/java/Deadlines.java
Outdated
* Deadlines tasks that need to be done before a specific date/time | ||
* e.g., submit report by 11/10/2019 5pm | ||
*/ | ||
|
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.
Consider, perhaps, deleting this line break between the comments and the function for consistency
src/main/java/Duke.java
Outdated
private static final ArrayList<Task> masterList = new ArrayList<>(); | ||
/** | ||
* Prints line break. | ||
* @return void |
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.
Consider adding a line break between the description of a function and its associated Javadoc tags for consistency. I noticed the same issue across other functions as well.
src/main/java/Duke.java
Outdated
private static final ArrayList<Task> masterList = new ArrayList<>(); | ||
/** | ||
* Prints line break. | ||
* @return void |
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 the @return tag here be deleted? It is not required here as per Oracle guidelines since the return type is void. Do refer to Javadoc guidelines for a more thorough reference for the conventions with respect to Javadoc comments.
src/main/java/Duke.java
Outdated
* @throws java.io.IOException If an I/O error occurs. Only takes in string. | ||
*/ | ||
private static final void printList(BufferedWriter bw) throws Exception { |
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.
Nice job on catching this! Do consider throwing an explicit IOException if you mention it as such in the documentation for the code though
src/main/java/Duke.java
Outdated
* @throws java.io.IOException If an I/O error occurs. Only takes in string. | ||
*/ | ||
private static final void printList(BufferedWriter bw) throws Exception { | ||
for(int i = 0; i < masterList.size(); i++) { |
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.
You might want to include a whitespace after the 'for' keyword here?
src/main/java/Duke.java
Outdated
} | ||
|
||
private static final String getDateTime(String[] inputArr) { | ||
return inputArr[1].split("/")[1].split(" ", 2)[1]; // split input from slash |
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.
Perhaps include the comment as a same-level indent above the line as opposed to an inline one? I noticed this in other places as well.
Since this function consists of a single line at the moment, you might want to add Javadoc-style documentation instead.
src/main/java/Duke.java
Outdated
BufferedReader br = new BufferedReader(new InputStreamReader(System.in)); | ||
BufferedWriter bw = new BufferedWriter(new OutputStreamWriter(System.out)); | ||
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
printLineBreak(); | ||
System.out.println("Hello! I'm Duke\nWhat can I do for you?"); | ||
String input; // to store raw input command | ||
String[] inputArr; // to store split input command | ||
boolean ifBye = false; | ||
do { | ||
printLineBreak(); | ||
System.out.println(); | ||
input = br.readLine(); | ||
inputArr = input.split(" ", 2); // split first word from body | ||
printLineBreak(); | ||
switch (inputArr[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.
Do consider separating the code into logical blocks with line breaks for ease of viewing. For instance, here, the do-while loop can be separated from the rest of the preceding lines, and the initialization of variables (e.g. BufferedReader) can be separated in a similar fashion as well.
src/main/java/Duke.java
Outdated
case "bye": | ||
ifBye = true; | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
break; | ||
|
||
case "list": |
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 job on following the guidelines with respect to the indentation levels of switch and case! There should be no need for line breaks between individual case statements, so maybe remove these as a final step?
LeDuke
FAST, LITE,
POWERFULMEGA STONKSLeDuke will help you with:
What? Did I hear that you want LeDuke? Follow these easy steps 👀:
Progress:
You can also use look for the
main
in my code: