-
Notifications
You must be signed in to change notification settings - Fork 200
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
[Wang Silang] iP #214
base: master
Are you sure you want to change the base?
[Wang Silang] iP #214
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, great job structuring the code. Some minor style issues.
src/main/java/Duke.java
Outdated
System.out.println("Sir, your task has been unmarked as requested."); | ||
} | ||
|
||
public static void printCurrentList(Task[] list, int numOfItems) |
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 making clear method and variable names!
src/main/java/Duke.java
Outdated
} | ||
|
||
public static void addList() | ||
{ |
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 (as done in the exitLine method)
src/main/java/Duke.java
Outdated
if(line.startsWith("todo")){ | ||
newTask = new ToDos(line); | ||
} | ||
else if(line.startsWith("deadline")){ |
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.
change the form of the if/else statements to match the coding standard:
if (condition) {
statements;
} else if (condition) {
statements;
} else {
statements;
}
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.
Yeah, you can refer to this link [https://se-education.org/guides/conventions/java/basic.html] for if-else statement coding standards.
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.
Thanks guys!
src/main/java/Duke.java
Outdated
@@ -1,3 +1,8 @@ | |||
import jdk.jfr.Event; |
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.
where/how is this used?
src/main/java/Duke.java
Outdated
if(line.startsWith("todo")){ | ||
newTask = new ToDos(line); | ||
} | ||
else if(line.startsWith("deadline")){ |
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.
Yeah, you can refer to this link [https://se-education.org/guides/conventions/java/basic.html] for if-else statement coding standards.
src/main/java/Duke.java
Outdated
{ | ||
String line; | ||
Scanner in = new Scanner(System.in); | ||
line = in.nextLine(); | ||
while(!line.equals("bye")) { | ||
System.out.println(line); | ||
line = in.nextLine(); | ||
} | ||
} |
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'd want to indent the { } properly, like:
public static void echo(){
// your code
}
src/main/java/Task.java
Outdated
@@ -0,0 +1,31 @@ | |||
public class Task { | |||
protected String taskLabel; |
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 how you have added another variable called taskLabel.
This is actually clear while storing the data in it instead of explicitly typing [D] / [E] / [T] every time while printing the lists.
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.
Your indentation is really good. StringBuffer
class is really useful, I like the way you use it to deal with strings. Maybe you can replace some "magic numbers" with constants to make your code more readable? Just some personal ideas.
src/main/java/Deadlines.java
Outdated
private String endTime; | ||
private String taskLabel = "[D]"; | ||
public Deadlines (String input){ | ||
super(input.substring(9,input.indexOf('/') - 1)); // Sanitize input by removing "deadline" at the start |
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.
Personally, maybe you can replace the number 9
with a constant to avoid "magic numbers"?
Like
private static final int DEADLINE_LABEL = 9;
?
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.
Agreed. Thanks for the idea!
src/main/java/Deadlines.java
Outdated
super.setTaskLabel(taskLabel); | ||
endTime = "(" + getEndTime(input) + ")"; | ||
} | ||
// Method of StringBuffer operation taken from |
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 way you use StringBuffer
to operate on these strings, it's really useful.
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.
Great Job! A few nits here and there. Do take a look at my comments below.
src/main/java/Duke/Duke.java
Outdated
public static void echo() { | ||
String line; | ||
Scanner in = new Scanner(System.in); | ||
line = in.nextLine(); | ||
while (!line.equals("bye")) { | ||
System.out.println(line); | ||
line = in.nextLine(); | ||
} | ||
} |
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 can remove methods that are not being used to reduce redundant code and improve readability.
src/main/java/Duke/Duke.java
Outdated
} | ||
line = in.nextLine(); // read in next line of text | ||
} | ||
} |
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.
Try to have less that 30 LOC for any method. You can split the contents of this method into multiple methods.
src/main/java/Duke/Duke.java
Outdated
System.out.println("Glad I could be of help!"); | ||
} | ||
|
||
public static void addList() { |
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 remember to provide appropriate names to methods. The method below is not just adding a list or adding to a list.
} | ||
return 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.
Good job refactoring the code!
@@ -0,0 +1,4 @@ | |||
package Duke.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.
Names representing packages should be in all lower case. Look at Java coding standards for more details.
branch-level-7
branch-A-JavaDoc
* tag 'branch-Level-9': branch-Level-9
JAR released
* master: branch-A-JavaDoc # Conflicts: # src/main/java/Duke/Ui.java
branch-level-9
No description provided.