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

[W5][T09-3]Rachel Lim #53

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

Conversation

rachellim10
Copy link

Added clock function that prints date and time

@arhjaye
Copy link

arhjaye commented Feb 11, 2019

Would be good to have a test file to ensure output is as expected for new function

Copy link

@stephlewyh stephlewyh left a comment

Choose a reason for hiding this comment

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

Hi Rachel,

Good job this time!

However, you could write unit tests to ensure that your program prints out the correct datetime format. Also you need to ensure that header comments are added to methods to inform the reader of its functionality.

@@ -156,6 +156,13 @@ Views all details of the 1st person in the results of the `find` command.
Clears all entries from the address book. +
Format: `clear`

== Prints out current date and time : `clock`
Format: `clock`

Choose a reason for hiding this comment

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

What is the description for your feature? It is missing.

+ ": Prints date and time in format: day, date-month-year at hr:min:sec AM/PM timezone.\n"
+ "Example: " + COMMAND_WORD;

@Override

Choose a reason for hiding this comment

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

Missing header comment. All non-trivial methods should have java doc format header comments.

public CommandResult execute() {
Date now = new Date();
SimpleDateFormat dateFormatter = new SimpleDateFormat("E, dd-MM-yyyy 'at' hh:mm:ss a z");
//System.out.println("Date & Time now: " + dateFormatter.format(now));

Choose a reason for hiding this comment

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

This comment is not necessary for your program to run, nor does it improve readability of your code.

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.

4 participants