boonhaii / pe

0 stars 0 forks source link

Find Section in Developer Guide #13

Open boonhaii opened 2 years ago

boonhaii commented 2 years ago

In the developer guide, the section for the command "Find" is not well explained sufficiently for the reader to understand how it is being implemented. While a sequence diagram is provided, it is not explained thoroughly and thus hard to check the validity of the diagram.

Thus, I am assuming that in the sequence diagram, FindCommand shouldn't be referenced once CommandResult is returned, and CollectivePredicate is not referenced after the method for #updateFilteredTuteeList is returned. Thus, both of the objects should be destroyed, and is currently not indicated in the diagram.

Screenshot 2021-11-12 at 5.36.19 PM.png

nus-pe-bot commented 2 years ago

Team's Response

"Find" is not well explained sufficiently for the reader

The sequence diagram extracted out the main interactions between the different classes. It serves to complement specifically the interactions that we mentioned in the Current Implementation section

Thus, both of the objects should be destroyed, and is currently not indicated in the diagram

We have clarified on the usage of object deletion before the PE, and the reply we got mentioned that object deletions are:

"Not a hard rule. In some cases, it might be important to show when an object is no longer referenced. But often, it can be omitted safely."

For the case of CollectivePredicate, we felt that there was not much value-add to the sequence diagram if we add the object deletion since we never included the low level methods such as test for CollectivePredicate, and hence left it out.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: While the team has stated that the object deletion is not a hard rule and can be safely omitted if necessary, it is weird that the FindCommandParser has the priority of having the object deletion notation while the FindCommand and CollectivePredicate have their object deletion notations omitted.

As such, rather than making the diagram less confusing for the reader by omitting the irrelevant details, I feel that the diagram has become more confusing, and thus believe that the team has unintentionally, rather than intentionally left out the object deletion notation for FindCommand.