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

[Bag Devesh Kumar] iP #502

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

Conversation

tensaida
Copy link

@tensaida tensaida commented Aug 29, 2022

Drake

She said, "Do you love me?"
I tell her, "Only partly"
I only love my bed and
managing my user's tasks, I'm sorry
– Drake, God's Plan

Drake the type of guy to free your mind of having to remember things you need to do. It's,

  • text-based
  • easy to learn
  • FAST SUPER FAST to use

All you need to do is,

  1. download it from here.
  2. double-click him.
  3. add your tasks.
  4. let him manage your tasks for you 😉

And it is FREE!

Features

  • Managing tasks
  • Managing deadlines
  • Reminders

If you're a Java programmer, you can use it to practice Java too. Here's the main method:

public class Main {
    public static void main(String[] args) {
        new Drake().run();
    }
}

damithc and others added 8 commits July 31, 2022 17:20
To make the UI testing work, I had to remove the logo.txt file since it contained non-ASCII symbols.
I added four Exception classes, supporting empty description, no date, unknown command errors
@tensaida tensaida changed the title Bag Devesh Kumar iP [Bag Devesh Kumar] iP Aug 29, 2022
Tasks are now saved to a file on the hard disk, which is retrieved in later runs of the program
# Conflicts:
#	src/main/java/Deadline.java
#	src/main/java/Event.java
Copy link

@wweqg wweqg left a comment

Choose a reason for hiding this comment

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

Overall, I feel that your program is pretty good, the structure is there and especially I like the way how you customize the response from Drake. 😄

However just some minor issues here. I notice that you write everything in your main method, which I guess, is not a good practice if you would like to build extension (include extra features). I guess you actually had a sense of it already as A-MoreOOP is one of this IP requirements to met.

Yeah so I think once you abstracted your code in main method into methods from multiple classes, your program would be kind of flawless (based on my understanding). 👀

I understand that this might take some time (personally I took one whole day to settle this OOP requirement), yup so if you need any help can always drop me a message. Also you could check out IP code from others on our IP dashboard to get a sense on how to organize different classes. 👍

public class Drake {

public static void main(String[] args) throws DrakeException, IOException {

Copy link

Choose a reason for hiding this comment

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

Hi I notice the exceptions here are being thrown instead of being handled. Would this terminate the program under some circumstances ? 🤔

Choose a reason for hiding this comment

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

Indeed! This may also cause file errors when your program terminates before your file is closed.

if (taskFile.createNewFile()) {
System.out.println("Task file created: " + taskFile.getName());
} else {
System.out.println("Task file already exists.");
Copy link

Choose a reason for hiding this comment

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

I think it is fine to not prompt this reminder. System.out.println("Task file already exists.")
Assuming the file is there, and every time after the user entered command, he/she will see

......
.....
(User enetered command)
Task file already exists.
........

I guess this will left user to wonder what does this reminder even mean 😕

event.markAsDone();
}
list.add(event);
break;
Copy link

@wweqg wweqg Aug 31, 2022

Choose a reason for hiding this comment

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

Would it be better if you named these numbers into constants ? 🤔 For example:

final int TASK_TYPE = 0;
....
switch (taskParts[TASK_TYPE])
....

Also, there should not be any indentation for case. For example switch method would look like:

switch(number) {
case 1:
    ...
    break;
case2:
    ..
    break;
.....

Choose a reason for hiding this comment

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

Agreed! Perhaps considering changing these numbers into constants could improve readability and also reduce the possibility of an invalid entry into the variable!

Copy link

@SilasTSL SilasTSL left a comment

Choose a reason for hiding this comment

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

I think more OOP could be done. I love your funny console outputs!

firstWord = command.substring(0, firstSpace);
}
System.out.println("------------------------------------------------------");
switch (firstWord) {

Choose a reason for hiding this comment

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

switch and case should not be indented according to Java coding standard

case "delete": {
String[] commands = command.split(" ");
try {
int taskNo = Integer.parseInt(commands[1]);

Choose a reason for hiding this comment

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

Maybe it's clearer to name this variable as taskNumber instead of taskNo? I think this variable appears in other places too!

break;
}
case "event": {
String filtered = command.substring("event ".length());

Choose a reason for hiding this comment

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

I think filtered could maybe be more specific?

Copy link

@kavantan kavantan left a comment

Choose a reason for hiding this comment

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

Overall, I find the code relatively easy to read and understand. Although as mentioned in the specific comments, perhaps you might want to add JavaDocs whenever possible and possibly refactor your code into subclasses/methods, to make your code even more readable. Apart from that, there are minor violations of the code quality guidelines and coding standard conventions, but I'm sure those can be rectified very easily. All the best!

public String toString() {
return "[D]" + super.toString() + " (by: " + by.format(DateTimeFormatter.ofPattern("dd MMM yyyy")) + ")";
}
}

Choose a reason for hiding this comment

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

Small issue, but I believe your file is missing a newline at the bottom. I noticed the same issue in several other files too.


public class Drake {

public static void main(String[] args) throws DrakeException, IOException {

Choose a reason for hiding this comment

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

I feel that your main method is slightly lengthy, perhaps you could consider refactoring some of the functionalities into other methods/classes?

Choose a reason for hiding this comment

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

Indeed! Perhaps you could consider thinking of the program in terms of multiple tools working together to make it work for example the interpreter that makes sense of the user's commands, the UI that involves what the user interacts with, the task list to store tasks etc.

command = sc.nextLine();
}
taskFile.delete();
File updatedTaskFile = new File("data/tasks.txt");

Choose a reason for hiding this comment

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

I noticed there are quite a few instances of repeated variables being used such as 'data/tasks.txt' and the long chain of hyphens '-----'. Have you considered declaring them as global constants instead?

Comment on lines 1 to 2
import java.io.FileWriter;
import java.io.IOException;

Choose a reason for hiding this comment

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

I like that you followed the coding standards for your import statements 👍

isDone = true;
}

public void unmarkAsDone() {

Choose a reason for hiding this comment

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

Minor issue, but have you considered renaming this method to markAsUndone() instead? I feel that unmarkAsDone might potentially leave some room for ambiguity.

Copy link

@linuschancs linuschancs left a comment

Choose a reason for hiding this comment

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

Overall, your code for the most part was easy to understand and followed coding convention, perhaps just for the main method a consideration would be to abstract out the logic into more classes and methods rather than handling all the logic there! There were a few minor coding standard violations such as indentations however nothing too serious!

public class Drake {

public static void main(String[] args) throws DrakeException, IOException {

Choose a reason for hiding this comment

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

Indeed! This may also cause file errors when your program terminates before your file is closed.

event.markAsDone();
}
list.add(event);
break;

Choose a reason for hiding this comment

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

Agreed! Perhaps considering changing these numbers into constants could improve readability and also reduce the possibility of an invalid entry into the variable!


public Event(String description, String at) {
super(description);
this.at = LocalDate.parse(at);

Choose a reason for hiding this comment

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

A possible consideration would be to pass the entire String into the Event class and throw the errors from this class to be handled in the main class. This applies for your others tasks as well.

Also a possible scenario that occurs with the current implementation is what if users input an invalid formatted date/ random input as the date? What will happen when the input is parsed?


public class Drake {

public static void main(String[] args) throws DrakeException, IOException {

Choose a reason for hiding this comment

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

Indeed! Perhaps you could consider thinking of the program in terms of multiple tools working together to make it work for example the interpreter that makes sense of the user's commands, the UI that involves what the user interacts with, the task list to store tasks etc.

Comment on lines 53 to 55
if (taskParts[1].equals("X")) {
task.markAsDone();
}

Choose a reason for hiding this comment

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

You could perhaps consider abstracting this into your Task class!

Ui, Storage, Parser, *Command classes were created.
The task list is not persistent across restarts.

Do not delete the task file after every command.

This ensures the task file is not deleted and replaced with an empty file after a restart.
The classes are all unorganised inside one directory.

Divide into multiple packages as the number of classes.

This ensures better organisation of code.
TaskListTest was added to test the behavior of the TaskList class
The Drake entrypoint was modified to handle exceptions better according to the coding standard.
tensaida and others added 10 commits September 13, 2022 05:02
The JavaFX technology was used to implement the GUI.
The Deadline and Event constructors make certain assumptions about the format of the parameters.

In the future, changes to the Parser class may make the Deadline and Event classes behave unexpectedly.

Let's use assertions to make sure the parameters are in the correct format.

Using assertions allows us to document important assumptions that should hold true for the parameters in the constructors.
The fileToList() method assumes that the task file data is always correct.

The task file data may get corrupted.

Let's use a default branch to throw an exception in case inappropriate data is encountered.

Using an exception allows us to document unexpected behaviour.
Add a default branch in case statement
There is no support for tasks that need to be done within a certain period.

Adding support for such a task will enrich the feature set of the app.

Let's add support for managing tasks that need to be done withing a certain period.
Add support for Do within a period tasks
* Both sides of the conversation are displayed in the same format.
* There is no background image.

* As the conversation is between the user and the app, it is asymmetric in nature. So, different format for both sides of the conversation would help the user quickly differentiate the bot's replies and the user's inputs.
* A background image would make the app more visually appealing.

Let's
* display the user's inputs against a white background and the bot's replies against a purple background.
* add a translucent background image.

* A purple background was chosen because it matches the color of the bot's profile picture.
* A translucent background image was chosen because it aids in the visibility of the text.
* Drake uses only one emoji.
* Drake uses special characters.

* Drake's not the type of guy to use only a single emoji.
* He's also not the type of guy to use special characters in his welcome message.

Let's
* add an emoji to Drake's welcome message.
* remove special characters from his welcome message.
* The error message does not make sense in certain situations (e.g. when the user does not add an event time when creating an Event task).
* The task file is added to git.

* A more relevant error message enhances the UX.
* There is no purpose to tracking changes in the task file.

Let's
* modify the error message to be more relevant.
* add the task file to gitignore
If the output is more than five lines, it is not visible in the chat bubble.

This makes it impossible for users to see their long task lists in full.

Let's modify the fxml files and Window.java so that the chat bubble can accommodate more text.
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