-
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
[Ng Yong Jie] iP #74
base: master
Are you sure you want to change the base?
[Ng Yong Jie] iP #74
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.
Would be good to split the code into functions or classes :)
src/main/java/Duke.java
Outdated
|
||
//level 4 | ||
// list with 100 rows, 3 columns, 0 - TaskType, 1 - isDone, 2 - TaskName | ||
String[][] TaskList = new String[100][3]; |
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 avoid using magic literals
src/main/java/Duke.java
Outdated
} | ||
} | ||
while(true) { | ||
Scanner ScannerObject4 = new Scanner(System.in); |
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 start local variables with a lowercase letter
src/main/java/Duke.java
Outdated
String command = input.substring(0, input.indexOf(' ')); | ||
String content = input.substring(input.indexOf(' ') + 1); | ||
|
||
boolean isToDo = Objects.equals(command, "todo"); |
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 could be command.equals("todo")
instead
src/main/java/Duke.java
Outdated
continue; | ||
} else if (isUnmark) { //unmark task | ||
int TaskNumber = Integer.parseInt(content); | ||
TaskList[TaskNumber-1][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.
Please leave spaces on the left and right of operators (like +, -, etc)
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, try to update the implementation with the latest requirements as the IP is due soon. There were some coding standard violations and improvements to code quality that I have highlighted and suggested.
src/main/java/Duke.java
Outdated
} | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
|
||
// // level 2 & 3 |
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.
Delete dead code that is no longer in use.
src/main/java/Duke.java
Outdated
|
||
|
||
//level 4 | ||
// list with 100 rows, 3 columns, 0 - TaskType, 1 - isDone, 2 - TaskName |
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, and do not repeat the obvious in comments if it can already be inferred from the code.
Code Quality Guideline: Comment minimally but sufficiently
src/main/java/Duke.java
Outdated
|
||
//level 4 | ||
// list with 100 rows, 3 columns, 0 - TaskType, 1 - isDone, 2 - TaskName | ||
String[][] TaskList = new String[100][3]; |
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 using magic numbers, good to define what these numbers represent as constants like MAX_TASK_LENGTH
and MAX_NUMBER_OF_TASKS
Code quality guideline: Avoid magic numbers
src/main/java/Duke.java
Outdated
} | ||
} | ||
while(true) { | ||
Scanner ScannerObject4 = new Scanner(System.in); |
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.
As per the Java Coding Standard, variable names must be in camelCase.
src/main/java/Duke.java
Outdated
TaskList[ListCounter][2] = content; | ||
System.out.println("Got it. I've added this task:"); | ||
System.out.println("[" + TaskList[ListCounter][0]+"][" + TaskList[ListCounter][1]+"] " + TaskList[ListCounter][2]); | ||
System.out.println("Now you have "+ (ListCounter+1) +" tasks in your 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.
Avoid long methods, consider breaking down into smaller methods.
Code Quality Guideline: Avoid Long Methods
src/main/java/Duke.java
Outdated
TaskList[ListCounter][2] = content; | ||
System.out.println("Got it. I've added this task:"); | ||
System.out.println("[" + TaskList[ListCounter][0]+"][" + TaskList[ListCounter][1]+"] " + TaskList[ListCounter][2]); | ||
System.out.println("Now you have "+ (ListCounter+1) +" tasks in your 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.
Ensure that your operators are preceded and followed by whitespaces, as per the Java coding standard.
src/main/java/Duke.java
Outdated
System.out.println("Got it. I've added this task:"); | ||
System.out.println("[" + TaskList[ListCounter][0]+"][" + TaskList[ListCounter][1]+"] " + TaskList[ListCounter][2]); | ||
System.out.println("Now you have "+ (ListCounter+1) +" tasks in your list."); | ||
} else if (isDeadline) { // add deadline to 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.
As per the Java Coding Standard, ensure that comments are indented relative to their position in the code.
No description provided.