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

WIP: New comp features #113

Closed
wants to merge 8 commits into from
Closed

WIP: New comp features #113

wants to merge 8 commits into from

Conversation

HoWeiChin
Copy link

issue 65 fixed

@@ -26,7 +26,7 @@
* and returns an AddPersonCommand object for execution.
* @throws ParseException if the user input does not conform the expected format
*/
public AddPersonCommand parse(String args) throws ParseException {
public AddPersonCommand parse(String args) throws ParseException, java.text.ParseException {

Choose a reason for hiding this comment

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

Is parse exception different from java.text.parse.exception? If there is no difference, please remove the duplicate.

Copy link
Author

Choose a reason for hiding this comment

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

There's a difference. From what I understand from the code base, a pure parse exception captures invalid user inputs and it extends from IllegalValueException. While java.text.parse exception extends Exception and captures error related to format issues for the Date objects.

@@ -118,6 +119,8 @@ private Model initModelManager(Storage storage, ReadOnlyUserPrefs userPrefs) {
initialPersonData = new Data();
initialCompetitionData = new Data();
initialParticipationData = new Data();
} catch (ParseException e) {
e.printStackTrace();

Choose a reason for hiding this comment

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

i think if you catch the parse exception, you should send a message and reinitialize the data to:

initialPersonData = new Data();
initialCompetitionData = new Data();
initialParticipationData = new Data();

in the catch block

return false;

Choose a reason for hiding this comment

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

this can be shortened to a single line:

return startDate.equals(endDate) || startDate.dateObj.before(endDate.dateObj)

requireNonNull(date);
checkArgument(isValidDate(date.trim()), MESSAGE_CONSTRAINTS);
this.date = date.trim();
SimpleDateFormat format = new SimpleDateFormat(DATE_FORMAT);
dateObj = format.parse(date);
}

Choose a reason for hiding this comment

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

so this constructor is good.

but what you want to do for the tests is that you want to create another CustomDate constructor:

public CustomDate(Date dateObj) {
requireNonNull(dateObj);
// rest of code
}

that doesnt throw a parse exception, so that you dont have to trhow the parseexceptions all over the code base especially in the tests and sample data util

@ooimingsheng ooimingsheng changed the title New comp features WIP: New comp features Nov 4, 2019
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.

None yet

2 participants