search

crypto-code / pe

0 stars 0 forks source link

Inaccurate Sequence Diagram [Page 6] #6

Open crypto-code opened 2 years ago

crypto-code commented 2 years ago

image.png

Here when delete is called, there should also be a call to the model.commitAddressBook() method since delete is an undoable command.

nus-pe-bot commented 2 years ago

Team's Response

We understand the concerns regarding this issue raised. However, we crafted this sequence diagram considering that this sequence diagram must be applicable to all XYZCommands. If we were to include commitAddressBook() call, we would be alienating the commands that cannot be undone. Thus, we opted for a sequence diagram that does not include the call to commitAddressBook() in order to preserve a common architecture that applies to all commands.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: This is still an issue since as this may cause confusion to the reader.

As stated under the undo command implementation:

image.png

Now when they look at the sequence diagram in question, they expect to see this call, since it calls the delete function similar to the example given.

image.png

But in the sequence diagram they don't see this which causes a confusion.

As you stated, that this is a general diagram for XYZ command, so I believe that there should at least be two separate sequence diagrams. One for undoable commands and one for non-undoable commands. Additionally, if you can state the list of commands covered by undo and redo, it would be helpful for the reader.

:question: Issue severity

Team chose [severity.Low] Originally [severity.Medium]

Reason for disagreement: > severity.Medium : A flaw that causes occasional inconvenience to some readers but they can continue to use the product.

This discrepancy between the undo command example and the sequence diagram observed for the same command would cause confusion to any developer reading this DG and this is not a rare occurrence.