-
Notifications
You must be signed in to change notification settings - Fork 426
[Zayd Mahmud] ip #466
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
base: master
Are you sure you want to change the base?
[Zayd Mahmud] ip #466
Conversation
This reverts commit a6aa641.
…k lists. Modify the Task class to include an abstract method to act as a toString for the file and to include a protected variable that can modify the completion status. Modify the Monty class to accommodate.
Add function to be able to retrieve the events or deadlines due/relevant on a specific date.
LuBolin
left a comment
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.
Mostly stuck with the conventions, good job!
The only thing that "needs" to be changed seems to be the .* import, and potentially having braces around one-liner if statements.
Good job!
src/main/java/Storage.java
Outdated
| @@ -0,0 +1,69 @@ | |||
| import java.io.*; | |||
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 import be explicit, instead of using .*?
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're right. It was supposed to be explicit, but I forgot to remove the wildcard after testing.
src/main/java/Monty.java
Outdated
| break; | ||
|
|
||
| case "delete": | ||
| if (argument.isEmpty()) throw new MontyException(" Your task number is out of range! How am I to keep track of my-- I mean, your -- beautiful 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.
Maybe we can wrap the throw in braces?
src/main/java/Event.java
Outdated
|
|
||
| @Override | ||
| public String toString() { | ||
| return "[E]" + super.toString() + " (from: " + from.format(OUTPUT_FORMAT) + " to: " + to.format(OUTPUT_FORMAT) + ")"; |
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 a line break would help with readability?
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.
Thanks for the suggestion. I wasn't entirely sure on where to place the separators for a line like this so I just left it as a single one.
src/main/java/Monty.java
Outdated
| Storage.saveTasks(tasks); | ||
| break; | ||
|
|
||
| default: |
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 that you include a default clause in your switch case 👍
Add Parser class to handle the command execution of the chatbot. Refactor Monty to work using new classes to better reflect the Single Responsibility Principle.
Add appropriate test classes to test modular and cohesive functionality. Modified Parser class to better handle exceptions.
# Conflicts: # src/main/java/monty/Monty.java # src/main/java/monty/storage/Storage.java
Edit Ui class to include message for the 'find' feature.
# Conflicts: # src/main/java/monty/parser/Parser.java
Follow git standard (to the best of my knowledge).
VikramGoyal23
left a comment
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.
LGTM, the only nitpicks I could find were in import statement formatting and wildcard imports, with some incorrect switch statement indentation here and there. I went ahead and marked all instances I could find.
Overall, I would say you've done a great job.
| import monty.exception.MontyException; | ||
| import java.util.ArrayList; |
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.
A line break between these imports would help improve readability since they belong to different packages.
|
|
||
| import monty.exception.MontyException; | ||
| import monty.storage.Storage; | ||
| import monty.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.
Wildcard imports are not allowed.
| import monty.ui.Ui; | ||
| import java.time.LocalDate; |
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.
A line break between these imports would help improve readability since they belong to different packages.
| switch (command) { | ||
| case "bye": { |
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.
Coding standard dictates that switch statements should have no indentation for their case clauses, outside of Lambda-style switch statements.
|
|
||
| private static void processDateCommand(String argument, ArrayList<Task> tasks, Ui ui) throws MontyException { |
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 putting header comments everywhere, but in my opinion, this private method is non-trivial enough to also warrant one.
| package monty.task; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import static org.junit.jupiter.api.Assertions.*; |
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.
Wildcard import.
| package monty.task; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import static org.junit.jupiter.api.Assertions.*; |
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.
Wildcard import.
| import org.junit.jupiter.api.Test; | ||
| import java.io.ByteArrayOutputStream; |
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.
Line break here would be good.
| import java.io.ByteArrayOutputStream; | ||
| import java.io.PrintStream; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.*; |
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.
Wildcard import.
| import org.junit.jupiter.api.Test; | ||
| import java.util.ArrayList; | ||
| import static org.junit.jupiter.api.Assertions.*; |
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.
Reorder the org.junit.jupiter imports to be together to follow the coding standard.
To better reflect the course's coding standards, I had to make changes to my IDE's settings, especially to handle switch statements and method braces. Line breaks were more complicated to handle and required case-by-case oversight. The coding standard will need to be maintained for future commits.
Refactor code to better reflect Java Coding Standards
Added a new feature to Monty that allows users to sort their task list. ToDo tasks can be sorted alphabetically, while deadlines can be sorted chronologically. In the case of events, the current functionality is to have them automatically sorted by their start dates, but other options are being considered.
Adding the BCD extension before pulling my previous PR changes to the origin repository led to some merge conflicts. Some functionality has been altered, but a rewrite will likely be necessary in future commits.
Previously, when sorting commands using todo, deadline and events, clusters of the different event types would be sorted correctly but they would not be sorted against each other. Under the current implementation, the sort command processes tasks in the order: todo --> deadline --> event (by start time). Therefore, the issue of only sorting clusters of each task type has been resolved.
The profile pictures were made larger. They are kept in square shapes to match the general aesthetic being considered. The aesthetic elements of the UI -- the background, text bar, font and dialog boxes were changed to look more visually interesting, with inspiration being derived from the game Final Fantasy VII.
Added a 'clear' as well as a 'confirm' function to enable the user to clear the task list in a single command after asking for confirmation. The delete command has also been modified to request a confirmation beforehand. Monty's response messages have been tweaked to add more personality to the bot for a more fun and engaging user experience. Some extra, unused pieces of code and some old JavaDoc comments have been removed to make the code look cleaner.
Comprehensive user guide has been updated, and issues involving gradle appear to have been resolved.
Made some small adjustments to the documentation to match Monty's behaviour more accurately. Updated Ui.png according to changes made.
The help command was added, which links to the product site where the different commands are listed and explained.
Monty -- The Obsessive Taskmaster 🤖
Monty, when he's not hounding on you for not adding to his list, is instead tending to it and curating it instead! Some of the things he can do for you include:
For those of you who are interested in programming, you should know that Monty likes to maintain himself in lists too! That's right, he's compartmentalised his internal mechanisms quite well. Take a look:
Note
Monty's design adhere's to object-oriented programming principles, which is a paradigm that views the world as a network of interacting objects. Each object has a state and a behaviour, but is never too complex! You can learn more about it [here.] (https://nus-cs2103-ay2425s2.github.io/website/schedule/week1/topics.html#W1-1)
Here are some working commands that can be used with Monty:
Some features Monty is expected to have by the end are: