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

PR for Week 4 peer review #543

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

Conversation

amosting
Copy link

@amosting amosting commented Sep 4, 2023

No description provided.

damithc and others added 27 commits August 9, 2023 02:16
Deleted Duke.java, replaced with Jarvis.java and edited Jarvis.java
Added logic to echo user commands.
If user enters anything other than bye, echo. If user enters bye, reply with goodbye message.
Added logic to store tasks and print out a list of tasks when the request "list" is given.
Added Task.java.
Added logic for "mark" and "unmark"
Added Todo.java, Event.java and Dealine.java which inherit from Task.java
Added logic in Jarvis.java to handle "todo", "event" and "deadline" commands
Added command line inputs into input.txt and expected outputs into expected.txt.
Added simple logic to handle erroneous inputs, this is not completed.
Removed ASCII art because it was causing too many issues with the input output comparison.
Added JarvisException.java which is the parent of IncorrectJarvisCommandException.java, InvalidTaskNumberException.java and WrongJarvisCommandFormatException.java
Added logic to handle errors in Jarvis.java
Added logic to handle delete command.
Added error handling for delete command.
Organised error handling to become more modular and added regex matching.
Added save logic to allow for task list to be saved to local drive.
Used gitignore on data.txt so that git will not track changes to data.txt as it is the file that stores the task list data.
Further modularised and cleaned up code in main.
This reverts commit 4aea4a8.
Added logic in Deadline class to handle date recognition.
Added empty function in Task class for inheritance in Deadline class.
TaskList.java added to extract out tasklist related code from main to keep code modular.
appendTask, addTask, deleteTask, getTask, countTask, isValidTaskNumber, printTask, printDelete, printTaskList all added into TaskList.java
Storage.java added to extract out loading and saving file related code from main to keep code modular.
load, deleteContents, save methods added
Logic for instantiating TaskList object from a saved file also added
Parser.java added to process user commands and execute the proper actions needed, this logic was shifted from Jarvis.java for OOP
Ui.java added to handle UI interfacing with users, printing the proper information needed for a pleasant user experience, this logic was shifted form Jarvis.java for OOP
Jarvis.java edited to handle the increase in OOP
Organised all .java files into the Jarvis Package
Organised classes into Jarvis package for better organisation
Changed the file name in the Gradle build file so that gradle can run the program.
Added ToDoTest and DeadlineTest classes and used JUnit to test the toString() methods ToDo and Deadline classes respectively.
Added archiveFileName in order for .jar file to be named correctly
Added findTask to TaskList class to identify matching tasks and return them in a Task ArrayList.
Added printFoundTask to Ui.java to show the user the found files.
Added JavaDoc descriptions to the Deadline, Event, Jarvis, Parser, Task, TaskList and ToDo classes.
/**
* Adds the list of valid commands into the validCommands ArrayList.
*/
private void addValidCommands() {

Choose a reason for hiding this comment

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

good job on naming the method as a verb and writing it in camelCase, keep it up!

boolean uncheckMatch = inputCommand.matches("unmark \\d+");
boolean todoMatch = inputCommand.matches("todo .+");
boolean deadlineMatch = inputCommand.matches("deadline .+ /.+");
boolean eventMatch = inputCommand.matches("event .+ /.+ /.+");

Choose a reason for hiding this comment

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

if possible try to use a prefix like is, has or was to make it clearer that it is a boolean

Copy link

@awhb awhb left a comment

Choose a reason for hiding this comment

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

Looks good 👍
Some cosmetic changes requested.

Comment on lines 86 to 89
System.out.println(line);
System.out.println(e.getMessage());
System.out.println(" There are currently " + this.taskList.size() + " tasks in the list.");
System.out.println(line);
Copy link

Choose a reason for hiding this comment

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

Do consider letting your implemented UI class handle rendering the text to the user!

* the basic information and operations related to a task.
* </p>
*/
public class Task {
Copy link

Choose a reason for hiding this comment

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

Do consider adding the abstract keyword to abstract classes and methods so it is more obvious to others as well!

Comment on lines 6 to 16
private static final String name = "Jarvis";
private static final String line = "____________________________________________________________";
private static final String greeting = "Good day Sir! I'm ";
private static final String question = "How can I help you today Sir?";
private static final String signOff = "Good bye Sir!";
private static final String listInforming = "Here are the tasks in your list Sir:";
private static final String markInforming = "Roger that Sir, I've marked this task as done:";
private static final String uncheckInforming = "Noted Sir, I've marked this task as NOT done yet:";
private static final String taskInforming = "As you please Sir, I've added the task:";
private static final String deleteInforming = "Alright Sir, I've removed this task";
private static final String findInforming = "Here are the matching tasks in your list Sir:";
Copy link

Choose a reason for hiding this comment

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

Given that these are meant to be constants, do consider naming them using the constant names style stated in the Java coding standard for this course!

Suggested change
private static final String name = "Jarvis";
private static final String line = "____________________________________________________________";
private static final String greeting = "Good day Sir! I'm ";
private static final String question = "How can I help you today Sir?";
private static final String signOff = "Good bye Sir!";
private static final String listInforming = "Here are the tasks in your list Sir:";
private static final String markInforming = "Roger that Sir, I've marked this task as done:";
private static final String uncheckInforming = "Noted Sir, I've marked this task as NOT done yet:";
private static final String taskInforming = "As you please Sir, I've added the task:";
private static final String deleteInforming = "Alright Sir, I've removed this task";
private static final String findInforming = "Here are the matching tasks in your list Sir:";
private static final String NAME = "Jarvis";
private static final String LINE = "____________________________________________________________";
private static final String GREETING_MESSAGE = "Good day Sir! I'm ";
private static final String QUESTION_MESSAGE = "How can I help you today Sir?";
private static final String SIGN_OFF_MESSAGE = "Good bye Sir!";
private static final String LIST_MESSAGE = "Here are the tasks in your list Sir:";
private static final String MARK_MESSAGE = "Roger that Sir, I've marked this task as done:";
private static final String UNCHECK_MESSAGE = "Noted Sir, I've marked this task as NOT done yet:";
private static final String TASK_MESSAGE = "As you please Sir, I've added the task:";
private static final String DELETE_MESSAGE = "Alright Sir, I've removed this task";
private static final String FIND_MESSAGE = "Here are the matching tasks in your list Sir:";

Added CheckStyle to detect coding style violations, added logic in build.gradle to support checkstyle.
Edited existing code after adding CheckStyle make sure coding style is not violated.
* </p>
*/
public class Deadline extends Task {
protected String by;
Copy link

Choose a reason for hiding this comment

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

These names seem to have the potential to confuse, perhaps a more descriptive name will make the differences between the two variables clearer?

* @param validInputCommand the valid command which the user inputted
*/
// identifies which command has wrong formatting and prints feedback to user
public static void isWrongFormat(String inputCommand, String validInputCommand) {
Copy link

Choose a reason for hiding this comment

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

The method name seems to imply that it will return a boolean, perhaps a more appropriate method name could be "handleWrongFormat"?

* </p>
*/
public class TaskList {
private ArrayList<Task> taskList;
Copy link

Choose a reason for hiding this comment

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

Perhaps a more appropriate name for this variable could be "tasks" to highlight that it stores a collection?

return isValid;
}

public ArrayList<Task> findTask(String keyword) {
Copy link

Choose a reason for hiding this comment

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

Since multiple tasks can be returned, perhaps a more appropriate name could be "findTasks"?

Added DialogBox, Launcher, Main and MainWindow classes to support javaFx.
Edited Jarvis, Parser, TaskList and Ui classes to support javaFx.
}
return null;
}
}

Choose a reason for hiding this comment

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

code seems to be a little convoluted, maybe try using enums to replace the if/else statements. For exception handling, i think it would be better if you create separate classes for the different exceptions instead of handling it using if/else

Added assertions to Jarvis, Parser, Storage, TaskList, Ui classes.
…asses.

Dead code and unused variables are redundant and confuses other developers.
Wrong use of static keyword confuses others developers as class level methods and attributes are not actually meant to be as such.

Delete dead code and remove unnecessary static keywords.

So as to declutter code and fix wrong class level methods.
Added gradle.yml file to set up CI for iP.
… and its children.

To support adding priority levels to tasks.
…tional priority value in Tasks.

The current pattern recognition logic does not support the additional priority string.
And the current constructors used to construct Task objects does not support Task classes.

New pattern recognition logic recognises when the user adds low/medium/high after typing the previous format of tasks.
…r input.

Edited regex expression in TaskList to handle priority field.
Edited return statements in Ui.java to return correct task format for new priority feature.
Edited README.md to have a proper user guide for Jarvis chatbot.
Edited some small Ui class return strings to ensure proper and consistent formatting.
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.

6 participants