-
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
[kristianachwan] iP #204
base: master
Are you sure you want to change the base?
[kristianachwan] iP #204
Conversation
src/main/java/Duke.java
Outdated
); | ||
String inputText; | ||
String outputMessage; | ||
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.
Should this method be divided? For example, mark method, unmark method can be seperated from this method.
src/main/java/Task.java
Outdated
this.isDone = true; | ||
} | ||
public void unmarkAsDone () { | ||
this.isDone = false; |
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.
Should this be indented?
src/main/java/Task.java
Outdated
@@ -0,0 +1,40 @@ | |||
public class Task { | |||
protected String description; |
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 you used protected variables instead of public!
src/main/java/Deadline.java
Outdated
this.deadlineBy = deadlineBy; | ||
} | ||
|
||
@Override |
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 you used @OverRide to make sure that you are properly overriding method from its parent
src/main/java/Task.java
Outdated
public class Task { | ||
protected String description; | ||
protected boolean isDone; | ||
static int numberOfTasks = 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 numberOfTasks can be protected variable too, so that it can only be acceessed by get method:)
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 the code works well I think, but it is quite cluttered and readability could be improved.
src/main/java/Duke.java
Outdated
while (true) { | ||
inputText = scanner.nextLine(); | ||
if (inputText.equals("list")) { | ||
outputMessage = Task.getTasksList(tasks); | ||
} else if (inputText.startsWith("mark")) { | ||
int taskIndex = getTaskIndexFromInput(inputText); | ||
tasks[taskIndex].markAsDone(); | ||
outputMessage = "Nice! I've marked this task as done: " + "\n\t\t" | ||
+ tasks[taskIndex].toString(); | ||
} else if (inputText.startsWith("unmark")) { | ||
int taskIndex = getTaskIndexFromInput(inputText); | ||
tasks[taskIndex].unmarkAsDone(); | ||
outputMessage = "Ok, I've marked this task as not done: " + "\n\t\t" | ||
+ tasks[taskIndex].toString(); | ||
} else if (inputText.startsWith("task")) { | ||
String taskDescription = inputText.split("/")[0].split("task")[1].trim(); | ||
Task newTask = new Task(taskDescription); | ||
tasks[Task.numberOfTasks - 1] = newTask; | ||
outputMessage = TASK_ADDED_PREFIX + newTask.toString() + "\n\t" | ||
+ getTaskAddedPostfix(); | ||
} else if (inputText.startsWith("todo")) { | ||
String toDoDescription = inputText.split("/")[0].split("todo")[1].trim(); | ||
Task newTodo = new ToDo(toDoDescription); | ||
tasks[Task.numberOfTasks - 1] = newTodo; | ||
outputMessage = TASK_ADDED_PREFIX + newTodo.toString() + "\n\t" | ||
+ getTaskAddedPostfix(); | ||
} else if (inputText.startsWith("deadline")) { | ||
String deadlineDescription = inputText.split("/")[0].split("deadline")[1].trim(); | ||
String deadlineBy = inputText.split("/")[1].trim(); | ||
Task newDeadline = new Deadline(deadlineDescription, deadlineBy); | ||
tasks[Task.numberOfTasks - 1] = newDeadline; | ||
outputMessage = TASK_ADDED_PREFIX + newDeadline.toString() + "\n\t" | ||
+ getTaskAddedPostfix(); | ||
} else if (inputText.startsWith("event")) { | ||
String eventDescription = inputText.split("/")[0].split("event")[1].trim(); | ||
String eventStart = inputText.split("/")[1].trim(); | ||
String eventEnd = inputText.split("/")[2].trim(); | ||
Task newEvent = new Event(eventDescription, eventStart, eventEnd); | ||
tasks[Task.numberOfTasks - 1] = newEvent; | ||
outputMessage = TASK_ADDED_PREFIX + newEvent.toString() + "\n\t" | ||
+ getTaskAddedPostfix(); | ||
} else if (inputText.equals("bye")) { | ||
outputMessage = "Bye. Hope to see you again soon!"; | ||
break; | ||
} else { | ||
outputMessage = "Sorry, I don't understand what you said. Can you say it again?"; | ||
} |
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.
Should these be extracted into different functions?
src/main/java/Task.java
Outdated
public static String getTasksList(Task[] tasks){ | ||
String tasksList = ""; | ||
for (int i = 0; i < numberOfTasks; i++) { | ||
tasksList += String.format("%3d. ", (i+1)) + tasks[i].toString() + "\n\t"; | ||
} | ||
return tasksList; | ||
} |
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.
Should this function be stored outside? Or the class be changed to a TaskList which includes the array of tasks?
src/main/java/Duke.java
Outdated
static Task[] tasks = new Task[100]; | ||
static final String TASK_ADDED_PREFIX = "Got it. I've added this task:\n\t"; |
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.
Should these variables be explicitly stated public/protected/private?
src/main/java/Task.java
Outdated
this.isDone = true; | ||
} | ||
public void unmarkAsDone () { | ||
this.isDone = false; |
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.
Should this line be indented?
src/main/java/Duke.java
Outdated
outputMessage = TASK_ADDED_PREFIX + newEvent.toString() + "\n\t" | ||
+ getTaskAddedPostfix(); | ||
} else if (inputText.equals("bye")) { | ||
outputMessage = "Bye. Hope to see you again soon!"; |
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.
Should this line (and other similar lines) have a newline added?
src/main/java/Deadline.java
Outdated
@@ -0,0 +1,15 @@ | |||
public class Deadline extends Task{ |
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.
Perhaps you can look into using the auto formatting feature in Intellij to standardise the spaces after the curly brackets.
src/main/java/Duke.java
Outdated
public class Duke { | ||
static Task[] tasks = 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.
Perhaps look into using an ArrayList might be better? It allows the array to be dynamically sized.
src/main/java/Duke.java
Outdated
String outputMessage; | ||
while (true) { | ||
inputText = scanner.nextLine(); | ||
if (inputText.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.
I feel that using a switch statement might make the code cleaner and easier to read.
src/main/java/Duke.java
Outdated
String toDoDescription = inputText.split("/")[0].split("todo")[1].trim(); | ||
Task newTodo = new ToDo(toDoDescription); | ||
tasks[Task.numberOfTasks - 1] = newTodo; | ||
outputMessage = TASK_ADDED_PREFIX + newTodo.toString() + "\n\t" |
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 on using UPPERCASE for constants.
Branch level 6
Add DataAccess class
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 compliance with code standards
please work on the object-orientedness of the code, currently most of the logic is contained in Duke which makes it hard to read and trace through.
try creating new classes for command, parser etc and refactor code accordingly.
refer to addressbook3 if required
printDuke(); | ||
String input; | ||
String outputMessage; | ||
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.
use condition here instead of system.Exit later
private static final String DUKE_PREFIX = "Duke Error: "; | ||
|
||
|
||
public static String[] parseCommand (String command) throws InvalidCommandException{ |
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 is there parseCommand in exception class?
Add find feature
Branch a java doc
No description provided.