[wongyihao0506] iP #183
Conversation
| System.out.println("OK, I've marked this task as not done yet:\n" + task); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
- Decent use of SLAP within your functions
- Inconsistent naming convention of variables at Line 9
- Remember to ensure consistency in spacing at Line 31 & 48
- It is advisable to create more macros for the 'magic' strings in your code
Betahaxer
left a comment
Betahaxer
left a comment
There was a problem hiding this comment.
Overall, the code looks like a good implementation of the chatbot. With some minor improvements, the readability and naming could be enhanced.
| protected String taskName; | ||
| protected boolean isDone; |
There was a problem hiding this comment.
Good naming of attributes eg isDone that explains what the boolean does
| public void markDone() { | ||
| isDone = true; | ||
| } | ||
| public void unmarkDone() { | ||
| isDone = false; | ||
| } |
| if (userInput.equals("bye")) { | ||
| System.out.println(Farewell); | ||
| break; | ||
| } else if (userInput.startsWith("mark ")) { | ||
| markList(userInput, true); | ||
| } else if (userInput.startsWith("unmark ")) { | ||
| markList(userInput, false); | ||
| } else if (userInput.equals("list")) { | ||
| showList(); | ||
| } else { | ||
| addToList(userInput); | ||
| System.out.println("added: " + userInput); | ||
| } |
There was a problem hiding this comment.
Could this logic be improved to reduce complexity? eg by using switch case
src/main/java/Growler.java
Outdated
| String greet = "Hello! I'm Growler \nWhat can I do for you? \n"; | ||
| String Farewell = "Bye. Hope to see you again soon!"; | ||
| System.out.println(greet); | ||
| String userInput; | ||
| Scanner in = new Scanner(System.in); | ||
| while (true) { | ||
| userInput = in.nextLine(); | ||
| if (userInput.equals("bye")) { | ||
| System.out.println(Farewell); | ||
| break; | ||
| } else if (userInput.startsWith("mark ")) { | ||
| markList(userInput, true); | ||
| } else if (userInput.startsWith("unmark ")) { | ||
| markList(userInput, false); | ||
| } else if (userInput.equals("list")) { | ||
| showList(); | ||
| } else { | ||
| addToList(userInput); | ||
| System.out.println("added: " + userInput); | ||
| } |
There was a problem hiding this comment.
Could the multiple levels of abstraction be avoided? ie can the greetings be abstracted too?
src/main/java/Growler.java
Outdated
| private static ArrayList<Task> list = new ArrayList<>(); | ||
| public static void main(String[] args) { | ||
| String greet = "Hello! I'm Growler \nWhat can I do for you? \n"; | ||
| String Farewell = "Bye. Hope to see you again soon!"; |
There was a problem hiding this comment.
I think there can be a better way to name this variable, like write it in camelCase.
src/main/java/Growler.java
Outdated
| int taskNumber = Integer.parseInt(userInput.split(" ")[1]) - 1; | ||
| Task task = list.get(taskNumber); | ||
|
|
||
| if(mark) { |
There was a problem hiding this comment.
Maybe you can have another name for the boolean, like isMarked.
src/main/java/Growler.java
Outdated
| private static void showList(){ | ||
| System.out.println("Here are the tasks in your list:"); | ||
| for (int i = 0; i < list.size(); i++){ | ||
| System.out.println((i+1) + "." + list.get(i)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Great job utilising loop for displaying the list with correct index and wrapping the statement by curly brackets despite there is only one statement.
src/main/java/Growler.java
Outdated
| private static void addTask(String description){ | ||
| list.add(new Task(description)); | ||
| } | ||
| private static void markList(String userInput, boolean mark){ |
There was a problem hiding this comment.
The naming of the methods are clear and concise, well done.
Hackin7
left a comment
Hackin7
left a comment
There was a problem hiding this comment.
do more SLAP and abstraction, generally methods are quite long (too long, exceeding 30 LoC)
| System.out.println(greet); | ||
| String userInput; | ||
| Scanner in = new Scanner(System.in); | ||
| while (true) { |
There was a problem hiding this comment.
Very bad case of arrow head code. I suggest doing more abstraction/ SLAP
src/main/java/Growler.java
Outdated
| } | ||
| in.close(); | ||
| } | ||
| private static void addTask(String userInput) { |
There was a problem hiding this comment.
This code is quite long, splitting it up into separate functions(per task) would be better
5 participants
No description provided.