Rename Logic#getFilteredPersonList()

[Zhiyuan-Amos]

Logic doesn't need to expose to the world that it uses a filtered list internally. Furthermore, the UI doesn't need to know what kind of list to bind itself too, as long as it is an ObservableList.

[yamgent]

What do you suggest we rename it to?

[Zhiyuan-Amos]

Not too sure, what do you suggest? :P How about Logic#getViewModel()? The header comments can be Returns an unmodifiable {@code ObservableList<Person>} for the UI to bind itself to. It seems reasonable for the Logic component to expose something about ViewModel since it is a high level component and it should expect that some UI component requires it?

[yamgent]

I think we did discuss about it before regarding how it feels rather weird to have a UI component being stored inside the Logic class, but we sort of let that issue through...

Anyway, by returning {@code ObservableList<Person>}, you are already exposing to the world that it uses filtered list internally. :P

[Zhiyuan-Amos]

I think we did discuss about it before regarding how it feels rather weird to have a UI component being stored inside the Logic class, but we sort of let that issue through...

This issue can be resolved by the next batch of interns :P Doesn't seem trivial to change it.

Anyway, by returning {@code ObservableList<Person>}, you are already exposing to the world that it uses filtered list internally. :P

A FilteredList is an ObservableList, but not the other way round :P So, we aren't really exposing that it uses a FilteredList internally (but it's kinda guessable though since we have FindCommand & ListCommand).

[pyokagan]

@Zhiyuan-Amos @yamgent Would it be possible to reboot this conversation and decide: (1) If this is really a big enough issue, and if so (2) what the replacement name will be?

[yamgent]

Logic doesn't need to expose to the world that it uses a filtered list internally.

From what I see there's two issues going on here:

1. getFilteredPersonList() internally uses a FilteredList but returns an ObservableList. I don't think that should be a concern: 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<Person>). The internal logic can implement this feature whatever way they want, with or without FilteredList,, and it would still be appropriate to call the method name getFilteredPersonList(). Or even getManipulatedPersonList(), but 'manipulated' is not a correct jargon to use.
2. Logic shouldn't store the filtered list: Whether that is desirable or not can be debated, but that would encompass more than just a renaming of the method name.

[pyokagan]

Furthermore, the UI doesn't need to know what kind of list to bind itself too, as long as it is an ObservableList.

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 :-P

I think that logic should just follow whatever the model named the list, unless logic is doing additional transformations on the list. Since the model named it getFilteredPersonList(), it should be named getFilteredPersonList() in the logic as well.

With that said, I do think that Model#getFilteredPersonList() should be renamed though -- since its the model it 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".

So, Model#getDisplayPersonList() and Logic#getDisplayPersonList()? Or is the user guide wrong?

[yamgent]

So, Model#getDisplayPersonList() and Logic#getDisplayPersonList()?

I think that is good.

[pyokagan]

I think that is good.

Well, some might complain that "displayed person list" is too biased towards visual user input interfaces. But really, it's just an abstract term. What's important is that the model matches the language used in the application domain.

@damithc Any thoughts? Shall we move forward with "displayed person list" as the application domain term?

[damithc]

I'm OK with the proposed names.

[pyokagan]

Alright, here is my re-take after a few months:

To start of: I agree that "filteredPersonsList" is not a very good name, but for a different reason: if students add more features (e.g. sorting of the persons list), they would need to rename it to "filteredSortedPersonsList" to be consistent, which would cause a lot of trouble. So, it is pretty important that it be renamed.

I now don't believe that "displayed person list" is a good name now, however, because it might give the wrong impression that it is in the model because it is being "displayed". Students may get the wrong impression that we are violating MVC here. It is perfectly possible to have a (perhaps command-line) UI that doesn't display the filtered person list at all, but the commands still operate on it. The actual concept of the "displayed persons list" transcends it being displayed, and I think it's pretty important to communicate that in the name.

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 :-P

Yeah, this statement is plain wrong. The UI can do useful stuff by binding itself to model.getAddressBook().getPersonList(), such as by displaying the total number of persons in the address book, independent of the current filter.

So, my current opinion is that the term "displayed persons list" is actually worse than the current "filteredPersonsList" name.

In terms of other names, I now offer up the name relevantPersons that is used in AB-2. It ticks the boxes of being generic (so it doesn't need to change if more transformations of the persons list are added), and it doesn't wrongly convey the impression that it exists because it will be displayed in the UI.

https://github.com/se-edu/addressbook-level2/blob/ffc79e3b7cada10c4ce8adf2a92e3dbe4e550305/src/seedu/addressbook/commands/Command.java#L49-L51

Thoughts?