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

DeveloperGuide.md: update find, clear, list person #187

Closed

Conversation

choiwab
Copy link

@choiwab choiwab commented Oct 31, 2024

Fixes #172

@choiwab choiwab added this to the v1.5 milestone Oct 31, 2024
@choiwab choiwab self-assigned this Oct 31, 2024
Copy link

codecov bot commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Copy link

@rxchell rxchell left a comment

Choose a reason for hiding this comment

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

LGTM, some changes to be made to reduce repetition.

Comment on lines +287 to +289
2. **Entity Type Verification**
`ListCommandParser#parse` trims the input to verify the entity type, checking for `PERSON_ENTITY_STRING` or `APPOINTMENT_ENTITY_STRING`.
If the entity type is `PERSON_ENTITY_STRING`, `ListCommandParser` returns an instance of `ListPersonCommand`.
Copy link

Choose a reason for hiding this comment

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

This is clear, but should we include it at the start of the section, since it applies to all the commmandparsers?

Copy link
Author

Choose a reason for hiding this comment

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

This too.

Comment on lines +253 to +254
1. **Input Parsing**
The user command input is parsed in `LogicManager` by calling `AddressBookParser#parseCommand`. For a command starting with "find person", the parser creates a `FindPersonCommandParser` instance to parse specific parameters.
Copy link

Choose a reason for hiding this comment

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

Should we remove this input parsing part for each feature, since it would be covered under the general implementation of the command at the start of the section? (you can refer to #186 for the general implementation of the entity commands, currently being worked on)

Copy link
Author

Choose a reason for hiding this comment

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

In general, I see that I may have to modify and remove redundant steps in implementation and design consideration. I will make appropriate changes as soon as we reach a consensus on the overall structure of our Feature part of DG.

Comment on lines +271 to +276
### 1. Predicate Composition
- **Reason**: Using predicates enables flexible filtering and reduces redundancy by allowing shared filtering logic across commands.

### 2. Case-Insensitive Matching
- **Reason**: User experience is enhanced by making searches case-insensitive.

Copy link

Choose a reason for hiding this comment

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

This is good!

Comment on lines +320 to +321
### 1. Abstracting Clear Logic
- **Reason**: The `ClearCommand` abstract class handles shared logic for clearing different entities (person or appointment).
Copy link

Choose a reason for hiding this comment

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

Should we move this to the start of the section, since the abstraction applies for all the commands?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, this abstract logic is common throughout all commands, we should consider making a general design consideration section.

@choiwab choiwab marked this pull request as draft November 6, 2024 18:48
@rxchell rxchell closed this Nov 6, 2024
@rxchell
Copy link

rxchell commented Nov 6, 2024

Fixed by #249

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.

Update DG: Find, List, Clear person
2 participants