Skip to content
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

[Brian Chow] iP #296

Open
wants to merge 51 commits into
base: master
Choose a base branch
from
Open

Conversation

brian16600
Copy link

@brian16600 brian16600 commented Jan 29, 2022

Duke

Duke is a interactive chatbot with UI.

"Your mind is for having ideas, not holding them" - David Allen

Duke is a task tracker that can track three categories of tasks:

  • Todo
  • Deadline
  • Event

Setting up

Setting up Duke is extremely easy. All you need to do is:

  1. Download JDK 11 and above from here
  2. Download the latest JAR file release from the right side of this repository
  3. Run the command java -jar ip.jar

It's that simple! :)

Commands

There are a few commands you can run in Duke. These include:

  • todo
  • deadline
  • event
  • delete
  • list
  • find
  • bye

Using an IDE

You can further edit/modify Duke by navigating to the src folder which contains all the java files

You can run it from Launcher.java by using main:

public class Launcher {
    public static void main(String[] args) {
        Application.launch(Main.class,args);
    }
}

damithc and others added 10 commits July 29, 2021 20:30
Added Event, Deadline and Todo subclasses of Task. Updated if-else in levelFourRespond to handle the 3 subclasses.
Created DukeException class to handle incorrect input length/commands. Shifted if-else loop to a switch-case in InputHandler.java,
Added recommended inputs for exceptions and added delete case in InputHandler.java.
Rewrote code into InputHandler.java. Added Storage.java to handle the storage of tasks as well as writing to data.txt file for saving.
Copy link

@declanleeler declanleeler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found the code easy to read and follow. There are a couple of coding standard violations. The InputHandler class is also very long and could be shortened with the use of more classes to split up the code in the switch-case and if-else.

* Represents a Task. Contains a Task constructor, two methods to mark and unmark tasks, toString() method as well as a isMark() method to check if Task is marked
*/
public class Task {
private boolean mark;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I like the naming of the boolean variable. As much as possible, use a prefix such as is, has, was, etc. for boolean variable/method names so that linters can automatically verify that this style rule is being followed. Perhaps it would be better to name it as follows?

Suggested change
private boolean mark;
private boolean isMarked;

/**
* markTask as done
*/
public void markTask () {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be named in this manner? Perhaps the following would comply with the coding standards better.

Suggested change
public void markTask () {
public void setMarkedTask () {

/**
* unmarkTask
*/
public void unmarkTask() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be named in this manner? Perhaps the following would comply with the coding standards better.

Suggested change
public void unmarkTask() {
public void setUnmarkedTask () {

*
* @return boolean on whether task is marked
*/
public boolean isMark() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be changed to another name, if you do decide to accept the suggestion of the boolean variable naming? Perhaps the following would work.

Suggested change
public boolean isMark() {
public void hasBeenMarked () {

String[] splitInput = input.split(" ");
String inputCommand = splitInput[0];
switch (inputCommand) {
case "todo":

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't like the indentation after the switch, are you sure this complies with the coding standard? Consider leaving no indentation for the case statements.

Changed multiple variable names to better suit their purposes. Added ability to take in specific date/time for Event/Deadline classes in yyyy-mm-dd hh:mm format and convert it. Added handling for DateTimeParseException
Copy link

@xMashedxTomatox xMashedxTomatox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty neat overall, LGTM! Just need to clean up a little on the javadocs and abstracting out certain logic to make the code a little bit better to read.

Copy link

@xMashedxTomatox xMashedxTomatox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks pretty neat overall, LGTM! Just need to clean up a little on the javadocs and abstracting out certain logic to make the code a little bit better to read.

*/
public class Deadline extends Task {
private String dueDate;
public Deadline(String name, String time) {super(name); this.dueDate = time;}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing javadocs for constructor.

Comment on lines 14 to 17
/**
* @override
* @return String of Deadline task, eg [D][X] Deadline (by:XX)
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Override should not be in the javadoc. Missing method's description for the javadoc.
Recommendation:

/**
* Prints the String representation for the Deadline task.
* 
* @return String of Deadline task, eg [D][X] Deadline (by:XX)
* /
@Override
public String toString() {...}

@@ -0,0 +1,12 @@
public class DukeException extends Exception{

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing javadoc for class.

* Processes the input into 7 categories: Todo, Event, Deadline, list, mark, unmark, bye and throws error
*/
public class InputHandler {
private ArrayList<Task> arr;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to have a more specific and plural name for a list of task.
Recommendation:

private ArrayList<Task> tasks;



/**
*

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing method description for javadoc.

Comment on lines 21 to 115
public boolean handleInput(String input) throws DukeException {
String[] splitInput = input.split(" ");
String inputCommand = splitInput[0];
switch (inputCommand) {
case "todo":
if (splitInput.length > 1) {
String[] nameArray = Arrays.copyOfRange(splitInput, 1, splitInput.length);
String name = String.join(" ", nameArray);
Todo newTodo = new Todo(name);
arr.add(newTodo);
printAddTaskMessage(newTodo);
return false;
} else {
throw new DukeException(":( OOPS!!! The description of a todo cannot be empty. Correct usage: todo [task]");
}
case "event":
if (splitInput.length > 3) {
String[] stringArrayExcludingEvent = Arrays.copyOfRange(splitInput, 1, splitInput.length);
String stringExcludingEvent = String.join(" ", stringArrayExcludingEvent);
String[] nameAndTimeArray = stringExcludingEvent.split("/at");
String name = nameAndTimeArray[0];
String time = nameAndTimeArray[1];
Event newEvent = new Event(name, time);
arr.add(newEvent);
printAddTaskMessage(newEvent);
return false;
} else {
throw new DukeException(":( OOPS!!! The description of a event cannot be empty. Correct usage: event [task] /at [time]");
}
case "deadline":
if (splitInput.length > 3) {
String[] stringArrayExcludingDeadline = Arrays.copyOfRange(splitInput, 1, splitInput.length);
String stringExcludingDeadline = String.join(" ", stringArrayExcludingDeadline);
String[] nameAndTimeArray = stringExcludingDeadline.split("/by");
String name = nameAndTimeArray[0];
String time = nameAndTimeArray[1];
Deadline newDeadline = new Deadline(name, time);
arr.add(newDeadline);
printAddTaskMessage(newDeadline);
return false;
} else {
throw new DukeException(":( OOPS!!! The description of a deadline cannot be empty. Correct usage: deadline [task] /by [time]");
}
case "list":
if (splitInput.length == 1) {
System.out.println("Here are the tasks in your list:");
int i = 0;
for (Task item : arr) {
i += 1;
if (item.isMark()) {
System.out.println(i + ". " + item);
} else {
System.out.println(i + ". " + item);
}
}
return false;
} else {
throw new DukeException("Wrong usage of list! Correct usage: list");
}
case "mark":
if (splitInput.length == 2) {
System.out.println("Nice! I've marked this task as done:\n");
int idx = Integer.parseInt(splitInput[1]) - 1;
Task taskToBeMarked = arr.get(idx);
taskToBeMarked.markTask();
return false;
} else {
throw new DukeException("Wrong usage of mark! Correct usage: mark [index]");
}
case "unmark":
if (splitInput.length == 2) {
System.out.println("OK, I've marked this task as not done yet:\n");
int idx = Integer.parseInt(splitInput[1]) - 1;
Task taskToBeUnmarked = arr.get(idx);
taskToBeUnmarked.unmarkTask();
return false;
} else {
throw new DukeException("Wrong usage of unmark! Correct usage: unmark [index]");
}
case "delete":
if (splitInput.length == 2) {
int idx = Integer.parseInt(splitInput[1]) - 1;
Task taskToBeDeleted = arr.get(idx);
arr.remove(idx);
System.out.println("Noted. I've removed this task:\n" + taskToBeDeleted + "\nNow you have " + arr.size() + " tasks in the list");
return false;
} else {
throw new DukeException("Wrong usage of delete! Correct usage: delete [index]");
}
case "bye":
return true;
default:
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time],"
+ " deadline [task] /by [time], mark [index], unmark [index], delete [index], bye");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method severely exceeds the recommended number of lines in a method (30).
Recommendation:
Can try to abstract out the details of each case statement into a different method.

public String name;

/**
* Constructor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be better to have a more specific description of a constructor instead of simply "Constructor".
Recommendation:

/**
* Creates a task object with the given parameter as the name of the task.
* @param name name of the task
*/

Copy link

@Dineshraj555 Dineshraj555 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, code was easy to read. Just minor coding standard violations to take down note of. Good job.

Comment on lines 11 to 12
private String dueDate;
public Deadline(String name, String time) {super(name); this.dueDate = time;}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could leave spacing for better readability.

Comment on lines 32 to 40









Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could clear by these empty spaces.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

*/
public class Event extends Task {
private String dueDate;
public Event (String name, String time) { super(name); this.dueDate = time;}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This do not follow the module coding standards.

case "bye":
return true;
default:
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time],"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add indentation.

import java.util.Arrays;

/**
* Represents a Task. Contains a Task constructor, two methods to mark and unmark tasks, toString() method as well as a isMark() method to check if Task is marked

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add indentation.

Copy link

@ianfromdover ianfromdover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat and organised codebase, variables are well named. Good job 👍

Comment on lines 28 to 32
/**
* @override
* @return String of Deadline task, eg [D][X] Deadline (by:XX) vs [D][✓] Deadline (by;XX)
*/

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps you might want to place the JavaDoc directly above the method without a spacing, as well as move the override outside for it to be in effect :)

Comment on lines 32 to 40









Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

Comment on lines 17 to 26
public Event (String name, String date) throws DateTimeParseException {
super(name);
this.dueDate = LocalDate.parse(date);
this.dueTime = null;
}
public Event (String name, String date, String time) throws DateTimeParseException {
super(name);
this.dueDate = LocalDate.parse(date);
this.dueTime = LocalTime.parse(time);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might want to include a line of space between the attributes and between methods :)

Comment on lines 34 to 96
switch (inputCommand) {
case "todo":
if (splitInput.length > 1) {
Todo newTodo = (Todo) parser.parse(CommandType.TODO, splitInput);
this.storage.writeData(newTodo);
printAddTaskMessage(newTodo);
return false;
} else {
throw new DukeException(":( OOPS!!! The description of a todo cannot be empty. Correct usage: todo [task]");
}
case "event":
if (splitInput.length > 3) {
Event newEvent = (Event) parser.parse(CommandType.EVENT, splitInput);
this.storage.writeData(newEvent);
printAddTaskMessage(newEvent);
return false;

} else {
throw new DukeException(":( OOPS!!! The description of a event cannot be empty. Correct usage: event [task] /at [time]");
}
case "deadline":
if (splitInput.length > 3) {
Deadline newDeadline = (Deadline) parser.parse(CommandType.DEADLINE, splitInput);
this.storage.writeData(newDeadline);
printAddTaskMessage(newDeadline);
return false;
} else {
throw new DukeException(":( OOPS!!! The description of a deadline cannot be empty. Correct usage: deadline [task] /by [time]");
}
case "list":
if (splitInput.length == 1) {
parser.parse(CommandType.LIST, this.storage, splitInput);
return false;
} else {
throw new DukeException("Wrong usage of list! Correct usage: list");
}
case "mark":
if (splitInput.length == 2) {
parser.parse(CommandType.MARK, this.storage, splitInput);
return false;
} else {
throw new DukeException("Wrong usage of mark! Correct usage: mark [index]");
}
case "unmark":
if (splitInput.length == 2) {
parser.parse(CommandType.UNMARK, this.storage, splitInput);
return false;
} else {
throw new DukeException("Wrong usage of unmark! Correct usage: unmark [index]");
}
case "delete":
if (splitInput.length == 2) {
parser.parse(CommandType.DELETE, this.storage, splitInput);
return false;
} else {
throw new DukeException("Wrong usage of delete! Correct usage: delete [index]");
}
case "bye":
return true;
default:
throw new DukeException(":( OOPS!!! I'm sorry, but I don't know what that means! Possible commands: todo [task], event [task] /at [time],"
+ " deadline [task] /by [time], mark [index], unmark [index], delete [index], bye");
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of switch statement to clean up code. Easy to follow 👍


public class Parser {

public Task parse(InputHandler.CommandType type, String[] splitInput) throws DukeException {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it may be easier to understand if the 2 parse functions were combined into 1 :)

Comment on lines 71 to 77
String mark = (task.hasBeenMarked()) ? "[✓]" : "[X]";
output = "[T] " + mark + " / " + task.name + "\n";
} else if (task instanceof Deadline deadline) {
String mark = (deadline.hasBeenMarked()) ? "[✓]" : "[X]";
output = "[D] " + mark + " / " + deadline.name + " / " + deadline.dueDate + " / " + deadline.dueTime + "\n";
} else if (task instanceof Event event) {
String mark = (event.hasBeenMarked()) ? "[✓]" : "[X]";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ticks are a nice touch 😊👌

Comment on lines 120 to 124
if (task.hasBeenMarked()) {
listOfTasks += i + ". " + task + "\n";
} else {
listOfTasks += i + ". " + task + "\n";
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of spaces between operators 👍

brian16600 and others added 26 commits February 13, 2022 01:08
Add Assertions for index out of bounds exception for delete/mark/unmark commands.

Had to handle this exception for user error, change made addresses this.

It is done using assertions to ensure index is within bounds.

Change some of the code behind the bye command and storage.

GUI was not exiting properly when bye was called, and some of the writing to storage had issues.

Add a new method rewriteData() to address this issue and fixed bye command.

Change storage writing for marked vs unmarked to 1(marked) vs 0(unmarked), since the GUI had issues displaying the tick.
Update and add documentation for all methods.

Improve abstraction throughout the code by abstracting out many Strings for return statements. Some methods reused the same String, hence abstracting them out makes the code cleaner.

Shorten the method length for methods in Parser.java, InputHandler.java and Storage.java as method lengths were too long and add new methods for each case in the switch to handle the code. This was aimed at making the code cleaner.
Add snooze command for the type B Extension. This functionality was added as an 8th command type.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants