lennoxtr / pe

0 stars 0 forks source link

Image of example of delete command is missing a return component #6

Open lennoxtr opened 1 year ago

lennoxtr commented 1 year ago

Descriptions:

image.png

In this image, a getEmployee(id) method is executed, but it returns nothing, and there is also no activation bar. I believe an employee object should be returned inorder for the deleteEmployee(e) to be executed.

Section affected:

3.3 Logic component

nus-se-bot commented 1 year ago

Team's Response

Hi, thanks for raising this. The omission is actually intentional to avoid clutter. We feel there is no loss in information being conveyed to the reader. That said, we have decided to add in the getEmployee(Id) method in case readers are wondering how an employee is identified and retrieved from SudoHR's database.

Further, if you recall from the textbook, it is explicitly stated that it is fine to omit activation bars and return arrows. Screenshot 2023-04-15 at 4.06.06 AM.png

Screenshot 2023-04-15 at 4.08.50 AM.png

The 'Original' Bug

[The team marked this bug as a duplicate of the following bug]

Sequence Diagram for commands missing crucial operations done between logic and model

Note from the teaching team: This bug was reported during the Part II (Evaluating Documents) stage of the PE. You may reject this bug if it is not related to the quality of documentation.


In DG, del command accesses model only using getEmployee(targetID), DeleteEmployee(toDelete) and refresh() methods

Missing Crucial Details.PNG

However, from the code, there are more methods that accesses model that are not reflected in the sequence diagram.

test.PNG

I have noticed that this problem recurs in most of the commands in DG (aetl, adep, edep etc.)


[original: nus-cs2103-AY2223S2/pe-interim#4211] [original labels: severity.Medium type.DocumentationBug]

Their Response to the 'Original' Bug

[This is the team's response to the above 'original' bug]

Hi, thank you for raising but we have intentionally omitted these methods from the sequence diagram because of the following:

  1. It is not essential to understanding the DeleteCommand, these methods are simply for ensuring the UI is refreshed
  2. Omitting them does not make the reader lose understanding of the logic behind DeleteCommand
  3. Adding all these methods would make the sequence diagram too cluttered and possibly unreadable, at which point it is possibly a bug due to readability issues
  4. We believe the refresh method indicated in the diagram sufficiently conveys the information that the UI will be refreshed.

It is even explicitly stated the goal of such diagrams is for comprehensibility and it is perfectly fine to omit less important details.

Screenshot 2023-04-15 at 2.13.55 AM.png

Below also shows the encouraged display of sequence diagrams (from CS2103T website Week 13), which is not at all too different. Screenshot 2023-04-15 at 4.11.08 AM.png

The same can be said for the other commands.

Items for the Tester to Verify

:question: Issue duplicate status

Team chose to mark this issue as a duplicate of another issue (as explained in the Team's response above)

Reason for disagreement: [replace this with your explanation]


## :question: Issue response Team chose [`response.Rejected`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]