FireRadical22 / pe

0 stars 0 forks source link

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

Open FireRadical22 opened 1 year ago

FireRadical22 commented 1 year ago

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.)

nus-se-script commented 1 year ago

Team's Response

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 response

Team chose [response.Rejected]

Reason for disagreement: [replace this with your explanation]


## :question: Issue severity Team chose [`severity.VeryLow`] Originally [`severity.Medium`] - [ ] I disagree **Reason for disagreement:** [replace this with your explanation]