-
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
[CHEN ZIHAN] ip #198
base: master
Are you sure you want to change the base?
[CHEN ZIHAN] ip #198
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 good job on the pull request! Only few minor consistency and coding standards issues.
src/main/java/Duke.java
Outdated
} | ||
|
||
|
||
|
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 the unnecessary blank spaces, follow one uniform spacing
src/main/java/Duke.java
Outdated
|
||
|
||
|
||
public static void add() { |
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.
Coding Standard violation: Ambiguity of function.
Can change the name of the function to explain more. e.g. addTodo()
src/main/java/Duke.java
Outdated
|
||
public static void add() { | ||
boolean isByeEntered = true; | ||
String outputs; |
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.
Coding Standard Violation: Plural form is reserved for collections of objects.
String outputs; | |
String output |
src/main/java/Duke.java
Outdated
public static void add() { | ||
boolean isByeEntered = true; | ||
String outputs; | ||
String[] storeList; |
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.
Coding Standard Violation: Array should be plural
String[] storeList; | |
String[] storeLists; |
src/main/java/Duke.java
Outdated
System.out.println("Bye. Hope to see you again soon!"); | ||
return; | ||
}else{ | ||
System.out.println(outputs); | ||
} else if(outputs.equals("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.
Coding Standard Violation: Give space before and after statement
} else if(outputs.equals("list")){ | |
} else if(outputs.equals("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.
same goes for line 30, 35
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, I like how your code is quite clean and you adhered to coding standards most of the time! I noticed some variable names and spacing that you may have unintentionally overlooked but they are just minor errors. Good job!
src/main/java/Duke.java
Outdated
|
||
boolean isByeEntered = true; | ||
String outputs; | ||
public static final int MAX_INT = 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.
Nice! I like your usage of constants (instead of magic numbers) and I like how they are typed (all uppercase, underscore to separate words).
src/main/java/Duke.java
Outdated
|
||
|
||
public static void add() { | ||
boolean isByeEntered = true; |
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.
Could isByeEntered
be named to sound more like a boolean variable? It is a bit confusing what isByeEntered
does just based off its name 😁 .
src/main/java/Duke.java
Outdated
if(list!=null) { | ||
System.out.println(a + ". " + list); | ||
a++; | ||
}else{ |
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.
Spacing is sometimes inconsistent (maybe by accident?). Here, }else{
has no spaces between the braces but elsewhere, there are spaces. I noticed this a few times in the above code too. Be sure to double check!
src/main/java/Duke.java
Outdated
public static void add() { | ||
boolean isByeEntered = true; | ||
String outputs; | ||
String[] storeList; |
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 your array specifier is attached to the type and not the variable 👍 nice
src/main/java/Duke.java
Outdated
String[] storeList; | ||
storeList = new String[MAX_INT]; | ||
int i = 0; | ||
int a = 1; |
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 wasn't sure on what a
did until I read the code fully... I think changing the variable name a
could make it more understandable for readers!
src/main/java/Duke.java
Outdated
case "Bye": | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
isByeEnter = true; | ||
break; |
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.
Please separate into different methods to handle different commands, such as displayAllTasks()
, UnmarkTask()
etc.
this up to level 2