-
Notifications
You must be signed in to change notification settings - Fork 116
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
[Ryan Tan] iP #103
base: master
Are you sure you want to change the base?
[Ryan Tan] iP #103
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, looks pretty ok! Incorporating Error Handling and Classes could save you a lot of headache in the future
src/main/java/Tommi.java
Outdated
int taskIndex = Integer.parseInt(input.substring(5)) - 1; | ||
markTask(taskIndex); | ||
} else if (input.startsWith("unmark ")) { | ||
int taskIndex = Integer.parseInt(input.substring(7)) - 1; | ||
unmarkTask(taskIndex); |
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.
Consider not having multiple levels of abstraction in a single code fragment
src/main/java/Tommi.java
Outdated
Scanner scanner = new Scanner(System.in); | ||
String input; | ||
|
||
while (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.
Maybe consider using a variable instead for readability
src/main/java/Tommi.java
Outdated
private static final String[] tasks = new String[MAX_TASKS]; // Array to store tasks | ||
private static final boolean[] isCompleted = new boolean[MAX_TASKS]; // Array to store task completion status | ||
private static int taskCount = 0; // Counter to keep track of the number of tasks |
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.
Why not use a Class to represent all instances of tasks?
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 concur
src/main/java/Tommi.java
Outdated
if (index >= 0 && index < taskCount) { | ||
isCompleted[index] = true; | ||
printLine(); | ||
System.out.println("Awesomesauce! I've marked this task as done:"); | ||
System.out.println(" [X] " + tasks[index]); | ||
printLine(); | ||
} |
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.
Consider including error handling
src/main/java/Tommi.java
Outdated
} | ||
|
||
private static void printLine() { | ||
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.
Consider using ( "_*30" ) instead
src/main/java/Tommi.java
Outdated
" ______ \n" | ||
+ "/_ __/__ __ _ __ _ (_)\n" | ||
+ " / / / _ \\/ ' \\/ ' \\/ / \n" | ||
+ "/_/ \\___/_/_/_/_/_/_/_/ \n" | ||
+ "____________________________________________________________\n" | ||
+ "Hello! I'm Tommi!\n" | ||
+ "What can I do for you?\n" | ||
+ "____________________________________________________________\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.
Indentation for wrapped lines should be 8 spaces (i.e. twice the normal indentation of 4 spaces) more than the parent line.
src/main/java/Tommi.java
Outdated
if (input.equals("bye")) { | ||
printExitMessage(); | ||
break; // Exit the loop | ||
} else if (input.equals("list")) { | ||
listTasks(); | ||
} else if (input.startsWith("mark ")) { | ||
int taskIndex = Integer.parseInt(input.substring(5)) - 1; | ||
markTask(taskIndex); | ||
} else if (input.startsWith("unmark ")) { | ||
int taskIndex = Integer.parseInt(input.substring(7)) - 1; | ||
unmarkTask(taskIndex); | ||
} else { | ||
addTask(input); | ||
} |
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.
Code would be much neater with a switch-case structure instead of if-else.
src/main/java/Tommi.java
Outdated
import java.util.Scanner; | ||
|
||
public class Tommi { | ||
private static final int MAX_TASKS = 100; | ||
private static final String[] tasks = new String[MAX_TASKS]; // Array to store tasks | ||
private static final boolean[] isCompleted = new boolean[MAX_TASKS]; // Array to store task completion status | ||
private static int taskCount = 0; // Counter to keep track of the number of tasks | ||
|
||
public static void main(String[] args) { | ||
printIntroMessage(); | ||
|
||
Scanner scanner = new Scanner(System.in); | ||
String input; | ||
|
||
while (true) { | ||
input = scanner.nextLine(); // Read user input | ||
|
||
if (input.equals("bye")) { | ||
printExitMessage(); | ||
break; // Exit the loop | ||
} else if (input.equals("list")) { | ||
listTasks(); | ||
} else if (input.startsWith("mark ")) { | ||
int taskIndex = Integer.parseInt(input.substring(5)) - 1; | ||
markTask(taskIndex); | ||
} else if (input.startsWith("unmark ")) { | ||
int taskIndex = Integer.parseInt(input.substring(7)) - 1; | ||
unmarkTask(taskIndex); | ||
} else { | ||
addTask(input); | ||
} | ||
} | ||
} | ||
|
||
private static void printIntroMessage() { | ||
String intro = | ||
" ______ \n" | ||
+ "/_ __/__ __ _ __ _ (_)\n" | ||
+ " / / / _ \\/ ' \\/ ' \\/ / \n" | ||
+ "/_/ \\___/_/_/_/_/_/_/_/ \n" | ||
+ "____________________________________________________________\n" | ||
+ "Hello! I'm Tommi!\n" | ||
+ "What can I do for you?\n" | ||
+ "____________________________________________________________\n"; | ||
System.out.println(intro); | ||
} | ||
|
||
private static void addTask(String task) { | ||
tasks[taskCount] = task; | ||
isCompleted[taskCount] = false; | ||
taskCount++; | ||
printLine(); | ||
System.out.println("added: " + task); | ||
printLine(); | ||
} | ||
|
||
private static void listTasks() { | ||
printLine(); | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < taskCount; i++) { | ||
String status = isCompleted[i] ? "[X]" : "[ ]"; | ||
System.out.println((i + 1) + "." + status + " " + tasks[i]); | ||
} | ||
printLine(); | ||
} | ||
|
||
private static void markTask(int index) { | ||
if (index >= 0 && index < taskCount) { | ||
isCompleted[index] = true; | ||
printLine(); | ||
System.out.println("Awesomesauce! I've marked this task as done:"); | ||
System.out.println(" [X] " + tasks[index]); | ||
printLine(); | ||
} | ||
} | ||
|
||
private static void unmarkTask(int index) { | ||
if (index >= 0 && index < taskCount) { | ||
isCompleted[index] = false; | ||
printLine(); | ||
System.out.println("OK, I've marked this task as undone:"); | ||
System.out.println(" [ ] " + tasks[index]); | ||
printLine(); | ||
} | ||
} | ||
|
||
private static void printExitMessage() { | ||
printLine(); | ||
System.out.println("Bye. Hope to see you again soon!"); | ||
printLine(); | ||
} | ||
|
||
private static void printLine() { | ||
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.
The code is good overall. It avoids long methods, deep nesting, complicated expressions and magic numbers. The code is well structured and easily understood.
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.
Looks good to me in general, just some nitpick
src/main/java/Tommi.java
Outdated
private static final String[] tasks = new String[MAX_TASKS]; // Array to store tasks | ||
private static final boolean[] isCompleted = new boolean[MAX_TASKS]; // Array to store task completion status | ||
private static int taskCount = 0; // Counter to keep track of the number of tasks |
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 concur
src/main/java/Tommi.java
Outdated
if (index >= 0 && index < taskCount) { | ||
isCompleted[index] = true; | ||
printLine(); | ||
System.out.println("Awesomesauce! I've marked this task as done:"); |
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 the personality added to the chatbot
src/main/java/Tommi.java
Outdated
String intro = | ||
" ______ \n" | ||
+ "/_ __/__ __ _ __ _ (_)\n" | ||
+ " / / / _ \\/ ' \\/ ' \\/ / \n" | ||
+ "/_/ \\___/_/_/_/_/_/_/_/ \n" | ||
+ "____________________________________________________________\n" | ||
+ "Hello! I'm Tommi!\n" | ||
+ "What can I do for you?\n" | ||
+ "____________________________________________________________\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.
You can consider using 8 spaces to split up your line as stated in the coding standard
src/main/java/Tommi.java
Outdated
@@ -0,0 +1,96 @@ | |||
import java.util.Scanner; |
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.
You can consider adding comments to help with readability
Use encapsulation to encapsulate input handler. Implement function abstraction in Tommi.java
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 think this is very concise and readable. The logic used is straightforward and effective too. In some places I have nits regarding edge case inputs which may crash the program, but according to the project specifications everything has been done very well!
src/main/java/Tommi.java
Outdated
private static void printIntroMessage() { | ||
String intro = """ | ||
______ \s | ||
/_ __/__ __ _ __ _ (_) | ||
/ / / _ \\/ ' \\/ ' \\/ /\s | ||
/_/ \\___/_/_/_/_/_/_/_/ \s | ||
____________________________________________________________ | ||
Hello! I'm Tommi! | ||
What can I do for you? | ||
____________________________________________________________ | ||
"""; | ||
System.out.println(intro); | ||
} |
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 appreciate the effort gone to customise your logo, and abstract it to a function for later use!
src/main/java/Deadline.java
Outdated
public class Deadline extends Task{ | ||
|
||
protected String by; | ||
|
||
public Deadline(boolean isDone, String taskName, String by) { | ||
super(isDone, taskName); | ||
this.by = by; | ||
} | ||
|
||
public Deadline updateIsDone(boolean newIsDone) { | ||
return new Deadline(newIsDone, this.taskName, this.by); | ||
} | ||
|
||
@Override | ||
public String toString() { | ||
return "[D]" + super.toString() + " (by: " + by + ")"; | ||
} | ||
} |
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 the immutability of the updateIsDone method and the clear toString representation. A small suggestion would be to validate the by field if it's supposed to follow a specific format (like a date or time). You might also want to add a method to update the deadline (by) in the future, just like you did for isDone. Otherwise, everything looks really well-structured!
src/main/java/InputHandler.java
Outdated
public static void processInputCases(String input) { | ||
if (input.equals("list")) { | ||
TaskList.listTasks(); | ||
return; | ||
} | ||
|
||
String[] words = input.split(" "); | ||
int taskIndex; | ||
|
||
switch (words[0]) { | ||
case "mark": | ||
taskIndex = Integer.parseInt(words[1]) - 1; | ||
TaskList.markTask(taskIndex); | ||
break; | ||
case "unmark": | ||
taskIndex = Integer.parseInt(input.substring(7)) - 1; | ||
TaskList.unmarkTask(taskIndex); | ||
break; | ||
case "todo": | ||
TaskList.addTask(new ToDo(false, input.substring(5))); | ||
break; | ||
case "deadline": | ||
String[] deadlineParts = input.substring(9).split("/by", 2); | ||
TaskList.addTask(new Deadline(false, deadlineParts[0], deadlineParts[1])); | ||
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.
I like the clear logic, use of switch and how compact everything is! One thing to consider is adding more error handling, especially around Integer.parseInt() and array indexing, to prevent crashes on invalid inputs. It might also be worth refactoring the splitting of strings into helper methods for better readability .
src/main/java/TaskList.java
Outdated
public class TaskList { | ||
|
||
private static final int MAX_TASKS = 100; | ||
|
||
private static final Task[] tasks = new Task[MAX_TASKS]; // Array to store tasks | ||
private static int tasksCount = 0; // Counter to keep track of the number of tasks | ||
|
||
public static void listTasks() { | ||
Tommi.printLine(); | ||
System.out.println("Here are the tasks in your list:"); | ||
for (int i = 0; i < tasksCount; i++) { | ||
System.out.println((i + 1) + ". " + tasks[i]); | ||
} | ||
Tommi.printLine(); | ||
} | ||
|
||
public static void addTask(Task task) { | ||
tasks[tasksCount] = task; | ||
tasksCount++; | ||
Tommi.printLine(); | ||
System.out.println("Sure. I've added the task: " + System.lineSeparator() | ||
+ task + System.lineSeparator() | ||
+ "There are now " + tasksCount + " tasks in the list."); | ||
Tommi.printLine(); |
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 class is well structured, and the methods are concise and really easy to understand 😄. Would you want to switch a dynamic data structure later down the track (i.e. ArrayList), to give you some scalability? It might also be good to add boundary checks for adding tasks to ensure it doesn’t exceed MAX_TASKS. Overall, great foundation!
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 in coding standard and code quality, just a few points for you to consider.
TaskList.markTask(taskIndex); | ||
break; | ||
case "unmark": | ||
taskIndex = Integer.parseInt(input.substring(7)) - 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.
Try to avoid magic literals(e.g. numbers, strings), you can consider using named constants to give the literals a meaning so that it will be easier to understand.
E.g.
static final double PI = 3.14236;
static final int MAX_SIZE = 10;
...
return PI;
...
return MAX_SIZE - 1;
src/main/java/tommi/TaskList.java
Outdated
+ task + System.lineSeparator() | ||
+ "There are now " + tasksCount + " tasks in the 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.
Please be consistent in placing the + sign. In InputHandler.java
file you have been putting the + sign at the back when you wrap lines. You can consider following course website e.g.
setText("Long line split"
+ "into two parts.");
if (isReady) {
setText("Long line split"
+ "into two parts.");
}
src/main/java/tommi/TaskList.java
Outdated
public static void markTask(int index) { | ||
if (index >= 0 && index < tasksCount) { | ||
Tommi.printLine(); | ||
System.out.println("Awesomesauce! I've marked this task as done:"); | ||
tasks[index] = tasks[index].updateIsDone(true); | ||
System.out.println(tasks[index]); | ||
Tommi.printLine(); | ||
} | ||
} | ||
|
||
public static void unmarkTask(int index) { | ||
if (index >= 0 && index < tasksCount) { | ||
Tommi.printLine(); | ||
System.out.println("OK, I've marked this task as undone:"); | ||
tasks[index] = tasks[index].updateIsDone(false); | ||
System.out.println(tasks[index]); | ||
Tommi.printLine(); | ||
} | ||
} |
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.
You can consider extracting these two functions further and pass in variables since they are pretty similar.
Branch level 9
Make Task an abstract class
JavaDoc comments
Minor code touch ups
Update User Guide
Handle first time opening file error caused by new save file not created
No description provided.