-
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
[Sebastian Soewanto] iP #205
base: master
Are you sure you want to change the base?
[Sebastian Soewanto] iP #205
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.
LGTM, just several naming proposals :)
Also, maybe consider switch case
instead of lots if else
statements. Some comments for explaining specific variables might also be nice.
Overall, it's fine in terms of code standard.
src/main/java/Duke.java
Outdated
public class Duke { | ||
public static void main(String[] args) { | ||
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
|
||
String line = "____________________________________________________\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.
Seems like it is a constant, so maybe consider using static final LINE_DIVIDER
(or some other similar name)?
src/main/java/Duke.java
Outdated
System.out.println("Hello from\n" + logo); | ||
System.out.print(line + "Hello! I'm Duke\n" + "What can I do for you?\n" + line); | ||
String[] list = new String[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.
I think you use this list to store the different tasks, so maybe it's better to name it as tasks
? :)
src/main/java/Duke.java
Outdated
String[] list = new String[100]; | ||
boolean[] isDone = new boolean[100]; | ||
String toFill; | ||
int listCount = 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.
Maybe something like tasksCount
can tell better on what you are counting here?
src/main/java/Duke.java
Outdated
System.out.print(line + "Hello! I'm Duke\n" + "What can I do for you?\n" + line); | ||
String[] list = new String[100]; | ||
boolean[] isDone = new boolean[100]; | ||
String toFill; |
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 think naming it checkerContent
or some other noun may better represent the meaning of the variable (verbs are more suitable to be used for the methods)
src/main/java/Duke.java
Outdated
else { | ||
toFill = " "; | ||
} | ||
System.out.println(i + 1 + ". [" + toFill + "] " + list[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.
Extracting i+1 to another line as i++
may make the code a little bit more clean)
src/main/java/Duke.java
Outdated
@@ -8,21 +8,39 @@ public static void main(String[] args) { | |||
+ "| |_| | |_| | < __/\n" | |||
+ "|____/ \\__,_|_|\\_\\___|\n"; | |||
|
|||
String line = "_________________________________\n"; | |||
String line = "____________________________________________________\n"; | |||
System.out.println("Hello from\n" + logo); | |||
System.out.print(line + "Hello! I'm Duke\n" + "What can I do for you?\n" + line); | |||
String[] list = new String[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.
Naming of ' String[ ] list ' is quite vague, maybe if possible to provide a more useful name for readability? e.g. ' String [ ] taskList '
src/main/java/Duke.java
Outdated
System.out.println(line + "Nice! I've marked this task as done:\n" + " [X] " + list[mark - 1]); | ||
System.out.print(line); | ||
} else if (input.startsWith("unmark")) { | ||
int unmark = Integer.parseInt(input.substring(7)); |
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.
The naming of ' unmark' is a bit confusing, maybe switch to ' taskToUnmark '?
src/main/java/Duke.java
Outdated
System.out.print(line); | ||
} else if(input.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.
Good indentation and presenting the conditional statements, which helps the code's readability.
src/main/java/Duke.java
Outdated
@@ -8,21 +8,39 @@ public static void main(String[] args) { | |||
+ "| |_| | |_| | < __/\n" | |||
+ "|____/ \\__,_|_|\\_\\___|\n"; | |||
|
|||
String line = "_________________________________\n"; | |||
String line = "____________________________________________________\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.
Naming can be changed to avoid confusion down the code block, could change to -> e.g. 'static final String margin' (since its a repeating string pattern to be printed out)
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.
Commented about code's quality
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.
src/main/java/Duke.java
Outdated
System.out.println("Hello from\n" + logo); | ||
System.out.print(line + "Hello! I'm Duke\n" + "What can I do for you?\n" + line); | ||
Task[] task = 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.
since the number of task can be more than 100. you can consider to use the dynamic size array list
src/main/java/Duke.java
Outdated
String logo = " ____ _ \n" | ||
+ "| _ \\ _ _| | _____ \n" | ||
+ "| | | | | | | |/ / _ \\\n" | ||
+ "| |_| | |_| | < __/\n" | ||
+ "|____/ \\__,_|_|\\_\\___|\n"; | ||
|
||
String line = "____________________________________________________\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.
Constant should be named by CAPITAL LETTER for example String DIVIDER, String LOGO
src/main/java/Duke.java
Outdated
System.out.println("Hello from\n" + logo); | ||
System.out.print(line + "Hello! I'm Duke\n" + "What can I do for you?\n" + line); | ||
Task[] task = 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.
task should be renamed to tasks (with s)
Updated class methods.
Up to level 3