Skip to content
This repository has been archived by the owner on Mar 5, 2023. It is now read-only.

Rename Model#getFilteredPersonList() and Logic#getFilteredPersonList() #1013

Closed
wants to merge 1 commit into from

Conversation

fzdy1914
Copy link
Contributor

Fixes #827.

We use method with name getFilteredPersonList() to get the list to
display in UI.

The method name should follow the terminology used by the application
domain. And our User Guide (which should be using terms from the
application domain since its meant for users) actually uses the term
"displayed person list".

Let's rename getFilteredPersonList() to getDisplayPersonList() to
follow the terminology.

We use method with name `getFilteredPersonList()` to get the list to
display in UI.

The method name should follow the terminology used by the application
domain. And our User Guide (which should be using terms from the
application domain since its meant for users) actually uses the term
"displayed person list".

Let's rename `getFilteredPersonList()` to `getDisplayPersonList()` to
follow the terminology.
@CanIHasReview-bot
Copy link

Click here to submit a new iteration when this PR is ready for review.

See this repository's contribution guide for more information.

@CanIHasReview-bot
Copy link

v1

@fzdy1914 submitted v1 for review.

(📚 Archive)

Checkout this PR version locally
git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/1013/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you wish to fetch this PR to.

@pyokagan
Copy link
Contributor

Ehh, I'm still on the fence about #827 . Let me think...

@fzdy1914 Do you personally think this is something that should be done?

@fzdy1914
Copy link
Contributor Author

@pyokagan Personally, I do not have a strong opinion of the name of this method. I think the previous name is also acceptable. So, it is not a must-need-done change.

@Zhiyuan-Amos
Copy link
Contributor

Comments from issue:

FilteredPersonList simply means that it is returning a list that can be indirectly manipulated by the users of the Logic API (through another method updateFilteredPersonList(Predicate).

The UI does need to know which list to bind itself to. The application won't be very useful if the UI bound itself to the model.getAddressBook().getPersonList() list instead

I agree with these comments. I'm wondering whether getFilteredPersonList() suggests that the list of persons can be filtered, which couples the Logic component to the existence of FindCommand (iirc, only FindCommand performs the filtering), thus justifying the renaming of this method to a more generic name getDisplayPersonList().

On a side note, I think we could write clearer header comments in Logic to suggest how this list can be modified.

@pyokagan
Copy link
Contributor

@Zhiyuan-Amos I replied in #827 (so we can keep the discussion in one place). I actually think the name "displayed person list" might be worse than just keeping it at the current status quo, despite the name "filtered person list" not being ideal.

@fzdy1914 fzdy1914 closed this Dec 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rename Logic#getFilteredPersonList()
4 participants