Bug in sequence diagram in Undo/Redo feature implementation in DG

[dohaduong]

According to your code base, the execute() function in LogicManager is only able to take String parameters execute(String command).

However, in the sequence diagram in Undo/Redo implementation for undo operation (page 19/35 in DG) shows that it will start by calling execute(undo) of LogicManager, which is not a valid function in LogicManager. I suppose it should be execute("undo") instead. Similarly for parseCommand(undo) in TeamBuilderParser - should be parseCommand("undo") instead.

[nus-pe-script]

Team's Response

Severity changed to Low(minor inconvenience):

As mentioned, both commands are missing " ". This is a typo error on my part.

While this is a typo, you have correctly pointed out the inconvenience in trying to use execute(undo) or parseCommand(undo) as they are invalid.

However, the main point of this diagram is to show UndoCommand's interaction with HistoryUtil, Memento and Originator. The interaction between LogicManager and any Command object has already been explained in the previous part on Logic.

Furthermore, there are no possible undo objects that can be mistaken as inputs in these fields. Thus, rather than an occasional inconvenience, this typo should be classified as a minor inconvenience instead.

Otherwise:

Accepted as a documentation bug.

Items for the Tester to Verify

:question: Issue severity

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

• [x] I disagree

Reason for disagreement: I disagree with the severity changed from Medium to Low.

I believe that the main use of sequence diagrams is to help future developers understand the code base without the burden of having to go through the code themselves, thus error made in this would cause occasional inconvenience to users, as they have to go through the code base to double check the sequence diagram. The bug does not "appear in very rare situation" - any future developer reading/using the DG would face this bug and thus, is not of Low severity.

I disagree the team's response on how this diagram only focuses on UndoCommand thus other bugs could be disregarded - this is inappropriate as any bugs in diagrams could cause occasional inconvenience to users. Moreover, in their previous part on Logic that they mentioned, their Delete sequence diagram is also not entirely correct - as mentioned by my #11 bug report (deletePerson(1) is incorrect, deletePerson() only takes in Person args, not int):

If comparing this diagram with the undo diagram mentioned above, users would be confused by how execute and parseCommand should be used. Thus, such inconvenience could cause inconvenience to most users and thus should be of Medium severity.

Moreover, I disagree with the team's assumption that "there are no possible undo objects that can be mistaken as inputs". Considering a future developer who is completely new to the code base, I believe that this bug has high likelihood of causing misunderstanding. The future developer could base on this diagram to develop the code base, thus this bug could result in more time spent on debugging and understanding the code base.

---