-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add unit tests for edit meeting and parser #124
Conversation
…into branch-EditMeeting
Branch edit meeting
Implement edit meeting
Implement edit meeting
Implement edit meeting
Branch edit meeting
Edit meeting
Edit meeting
Update EditMeetingCommand.java
Edit meeting
Add unit tests for edit meeting and parser
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #124 +/- ##
============================================
+ Coverage 72.59% 74.29% +1.69%
- Complexity 512 547 +35
============================================
Files 85 88 +3
Lines 1704 1832 +128
Branches 175 193 +18
============================================
+ Hits 1237 1361 +124
+ Misses 418 414 -4
- Partials 49 57 +8 ☔ View full report in Codecov by Sentry. |
Add unit tests for edit meeting and parser
public static final String MESSAGE_USAGE = COMMAND_WORD + ": Edits the details of the meeting identified " | ||
+ "by the index number used in the displayed meeting list. " | ||
+ "Existing values will be overwritten by the input values.\n" | ||
+ "Parameters: INDEX (must be a positive integer) " | ||
+ PREFIX_CLIENT_INDEX + "CLIENT INDEX " | ||
+ PREFIX_MEETING_INDEX + "MEETING INDEX " | ||
+ PREFIX_NAME + "NAME " | ||
+ PREFIX_DATETIME + "DATETIME \n" | ||
+ "Example: " + COMMAND_WORD + " " | ||
+ PREFIX_CLIENT_INDEX + "1 " | ||
+ PREFIX_MEETING_INDEX + "2 " | ||
+ PREFIX_NAME + "starbucks meeting " | ||
+ PREFIX_DATETIME + "01-01-2024 12:00 "; |
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 the format for the usage message conforms to the format of the other commands/
/** | ||
* Parses input arguments and creates a new EditCommand object | ||
*/ |
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 you meant EditMeetingCommand here?
/** | ||
* Creates and returns a {@code Person} with the details of {@code personToEdit} | ||
* edited with {@code editPersonDescriptor}. | ||
*/ |
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.
Remember to update the javadocs for your code!
/** | ||
* @param clientIndex of the person in the filtered person list to edit | ||
* @param meetingIndex of the meeting in the filtered meeting list to edit | ||
* @param editPersonDescriptor details to edit the person with | ||
*/ |
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 you meant to type meetingIndex and editMeetingDescriptor?
* @param meetingIndex of the meeting in the filtered meeting list to edit | ||
* @param editPersonDescriptor details to edit the person with | ||
*/ | ||
public EditMeetingCommand(Index clientIndex, EditMeetingDescriptor editPersonDescriptor, Index meetingIndex) { |
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 your variables could be named differently since it's the meeting we are editing, not the client.
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 the javadocs for this file could be rewritten and the variable names updated?
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 the javadocs could be updated for this file.
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, only left with a few javadocs to update and maybe some variable names could be more appropriate.
Add unit tests for edit meeting and parser
* @param meetingIndex of the meeting in the filtered meeting list to edit | ||
* @param editMeetingDescriptor details to edit the meeting with | ||
*/ | ||
public EditMeetingCommand(Index clientIndex, EditMeetingDescriptor editMeetingDescriptor, Index meetingIndex) { |
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 use of utility methods provided by address book to check for non null
* Stores the details to edit the meeting with. Each non-empty field value will replace the | ||
* corresponding field value of the meeting. | ||
*/ | ||
public static class EditMeetingDescriptor { |
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.
interesting way to approach the problem, could you explain your thought process for creating a static class and using optionals?
No description provided.