-
Notifications
You must be signed in to change notification settings - Fork 77
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
Shen Zheqi #59
base: master
Are you sure you want to change the base?
Shen Zheqi #59
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.
Goooooooooood WOWWWWWW
src/main/java/Duke.java
Outdated
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
System.out.println("Hello from\n" + logo); | ||
printWelcomeMessage(); | ||
Task[] taskLists = new Task[100]; |
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.
Plz avoid using the magic number~
src/main/java/Duke.java
Outdated
|
||
while(!userInput.equals("bye")){ | ||
String[] userInputSplit = userInput.split(" "); | ||
switch(userInputSplit[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.
Good job on using correct indent on Switch!
src/main/java/Task.java
Outdated
@@ -0,0 +1,26 @@ | |||
public class Task { | |||
private boolean isDone; | |||
String description; |
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.
Yea can add the public label on it~
src/main/java/Task.java
Outdated
String description; | ||
public static int numOfTasks = 0; | ||
|
||
public Task (String description){ |
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 getter toc control the access to attr!
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, almost. Just a few nits to fix.
src/main/java/Duke.java
Outdated
|
||
String userInput = getUserInput(); | ||
|
||
while(!userInput.equals("bye")){ |
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.
Explicit use of switch
src/main/java/Duke.java
Outdated
|
||
public static void printEchoInput(String userInput){ | ||
String echoInput = | ||
" ____________________________________________________________\n" |
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.
Maybe you can separate the print of the line
src/main/java/Duke.java
Outdated
System.out.println(" ____________________________________________________________"); | ||
System.out.println(" Here are the tasks in your list:"); | ||
String isDoneNotation; | ||
for (int i=0; i<count; 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.
i = 0; i < count , maybe more spaces
src/main/java/Duke.java
Outdated
public static void printWelcomeMessage() { | ||
String welcomeMessage = | ||
" ____________________________________________________________\n" | ||
+ " Hello! I'm Duke\n" |
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.
Maybe using System.lineSeparator() is better than \n
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! I have highlighted some coding standard violations and improvements to code quality.
src/main/java/Duke.java
Outdated
} | ||
|
||
private static void deleteTask(ArrayList<Task> taskLists, int deleteIndex) { | ||
System.out.println(" ____________________________________________________________"); |
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 avoid magic strings, if there are strings like this that need to be reused, it is good to create a constant called DIVIDER
and reusing it at every occurence to improve readability.
Code Quality Guideline: Avoid Magic Literals
src/main/java/Duke.java
Outdated
System.out.println(" Noted. I've removed this task:"); | ||
System.out.println(String.format(" %s", taskLists.get(deleteIndex).toString())); | ||
taskLists.remove(deleteIndex); | ||
Task.numOfTasks --; |
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 need to leave a space for post Increments as per the Java coding standard
src/main/java/Duke.java
Outdated
Task.numOfTasks ++; | ||
printEchoInput(newEvent); | ||
} | ||
// add 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.
This seems like a reminder to yourself to add exceptions. Avoid personal comments that are private notes to yourself, instead comments should be written to the reader to improve readability. If not necessary here, can remove it altogether.
Code quality guideline: Write to the Reader
src/main/java/Duke.java
Outdated
System.out.println(byeMessage); | ||
} | ||
|
||
// replace count |
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.
Write comments to the reader. Can remove unnecessary comments if they are not to the reader.
Coding Standard Guideline: Comment minimally, but sufficiently
src/main/java/Duke.java
Outdated
|
||
String userInput = getUserInput(); | ||
|
||
while(!userInput.equals("bye")){ |
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.
Since you have already declared the constant string BYE
, can be reused here
src/main/java/Duke.java
Outdated
deleteTask(taskLists, deleteIndex); | ||
break; | ||
|
||
default: |
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.
Default should have the same indent level as your other cases in switch
, as per the Java coding standard.
src/main/java/Duke.java
Outdated
printWelcomeMessage(); | ||
ArrayList<Task> taskLists = new ArrayList<Task>(); | ||
|
||
|
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.
Avoid long methods, try to break down methods longer than 30 lines of code into smaller methods.
Code Quality Guideline: Avoid Long Methods
Branch level 9
Branch a java doc
No description provided.