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

[Ramachandran Sandhya] Duke Increments #13

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

Conversation

sandydays
Copy link

Add Week 2 iPs

Copy link

@shiyao821 shiyao821 left a comment

Choose a reason for hiding this comment

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

Very intuitive and generally easy to understand code.

To be done:

  • Documentation
  • Tests
  • Adding Ui, Parser, Storage, TaskList classes (as per Wk 3 requirements)
  • Level-9 find command

} catch (DukeException ex) {
System.out.println(ex.getMessage());
} finally {
if (hasChanged) {

Choose a reason for hiding this comment

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

The current implementation of saving your tasks after every command is fine for now. However, this process may take a long time when the to-do list gets long. Perhaps save only when the 'bye' command is entered?

Choose a reason for hiding this comment

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

Hmm I think Sandy is right though. Specification from the [website](The specification on the website says ) - "Save the tasks in the hard disk automatically whenever the task list changes."

Copy link
Author

Choose a reason for hiding this comment

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

I think @shiyao821, your suggestion is definitely the more efficient one in terms of the number of times you write to your file in memory, but I guess as @larrylawl pointed out, the website seems to indicate otherwise. Right now, with all the GUI also set up, user can just hit the close button on the top right of the interface to exit from the application, and this can be problematic if we only save all changes to the task list when user enters command bye. So, I am going with writing the tasks in the file again each time the user changes the task list in any way.

protected String[] monthMappings = new String[]{"", "January", "February", "March", "April", "May", "June",
"July", "August", "September", "October", "November", "December"};

public DateAndTime(String dateAndTime) {

Choose a reason for hiding this comment

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

Documentation on parse-able dateAndTime strings will help improve code readability

Copy link
Author

Choose a reason for hiding this comment

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

I now have included Javadoc comments in later commits!


while (!userInput.equals("bye")) {
try {
String[] command = userInput.split(" ", 2);

Choose a reason for hiding this comment

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

This is smart.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks :)

@@ -0,0 +1,34 @@
public class DateAndTime {

Choose a reason for hiding this comment

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

Very intuitive way of processing date and time. I like it! Moving on, checks should be made to prevent invalid date/time inputs from being processed eg. 13/16/19 2460

Choose a reason for hiding this comment

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

I like the effort that you have put in!
Just sharing another POV - you can use the built-in type Date (find out more here), instead of creating your own class.

Choose a reason for hiding this comment

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

I just realised the parsing of the date is also handled here. You can use the built in class SimpleDateFormat to parse input strings, and also output it in the format you want.

Eg on parsing

String format = "dd/MM/yyyy HHmm";
SimpleDateFormat readFormat = new SimpleDateFormat(format);
readFormat.parse(//input string)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the inputs!


public Event(String description, String at) {
super(description);
this.at = new DateAndTime(at);

Choose a reason for hiding this comment

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

What if the input given by the user is not an intended date and time? eg. (at: Block 903/904)?

Copy link
Author

Choose a reason for hiding this comment

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

I have tried to, in my more recent commits, ensure the date and time is of the correct format. I throw an exception if it is not!


@Override
public String toString() {
return "[E]" + super.toString() + " (at: " + at + ")";

Choose a reason for hiding this comment

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

Calling the super class toString() here is pretty smart. I've never thought of it.

Choose a reason for hiding this comment

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

Agreed :)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks :)

@@ -0,0 +1,34 @@
public class DateAndTime {

Choose a reason for hiding this comment

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

I like the effort that you have put in!
Just sharing another POV - you can use the built-in type Date (find out more here), instead of creating your own class.

@@ -0,0 +1,34 @@
public class DateAndTime {

Choose a reason for hiding this comment

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

I just realised the parsing of the date is also handled here. You can use the built in class SimpleDateFormat to parse input strings, and also output it in the format you want.

Eg on parsing

String format = "dd/MM/yyyy HHmm";
SimpleDateFormat readFormat = new SimpleDateFormat(format);
readFormat.parse(//input string)

} catch (DukeException ex) {
System.out.println(ex.getMessage());
} finally {
if (hasChanged) {

Choose a reason for hiding this comment

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

Hmm I think Sandy is right though. Specification from the [website](The specification on the website says ) - "Save the tasks in the hard disk automatically whenever the task list changes."

@@ -0,0 +1,14 @@
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.

Good effort on creating your own Duke Exception!

Copy link
Author

Choose a reason for hiding this comment

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

Thanks :)


@Override
public String toString() {
return "[E]" + super.toString() + " (at: " + at + ")";

Choose a reason for hiding this comment

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

Agreed :)

sandydays and others added 28 commits September 11, 2019 14:09
Add assertions and fix minor bugs.
Improve code quality. Add more error handling.
…done by user, the next task is added to the task list. Up to user to delete any of the recurring tasks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants