search

drustanyjt / pe

0 stars 0 forks source link

Unclear what is returned in Seq Diagram #15

Open drustanyjt opened 5 months ago

drustanyjt commented 5 months ago

Documentation Bug Description

This is the StarCommand section Seq Diagram:

image.png

Notice it does not mention what is returned from StarCommand:

image.png

Expected

Say that it is the command result or the command that is being returned.

Severity

severity.Medium : A flaw that causes occasional inconvenience to some users, but they can continue to use the product.

severity.High : A flaw that affects most users and causes major problems for users. i.e., only problems that make the product almost unusable for most users should have this label.

Not a purely cosmetic issue in docs or UI, so cannot be VeryLow. Medium because all Devs would need to go into the code to figure out what exactly is going on, quite inconvenient and undermines the convenience of having diagrams.

nus-se-script commented 5 months ago

Team's Response

Hello, thank you for raising this. However we think that the issue is not as as severe as stated, since the omission of the return value doesn't the affect central idea of the diagram (i.e., not knowing what StarCommand returns does not greatly affect the understanding of the overall command execution sequence.)

Hence we are considering this to be a cosmetic issue instead.

Items for the Tester to Verify

:question: Issue severity

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

Reason for disagreement: I don't agree that this is purely cosmetic, because the omission of what is being returned, combined with the fact that CommandResult was left in the diagram, makes the diagram as a whole less clear, which affects the reader's ability to understand what's important in the diagram.

First take a look at this code snippet of StarCommand:

image.png

On line 69 we see that the CommandResult object that was created is being returned. So it is true that a CommandResult is being returned. This by itself is not a problem, because return values for sequence diagrams are optional. However looking at the diagram:

image.png

Notice that the instantiated CommandResult in the diagram is never used, nor referenced again at all in the whole diagram, or the text. Because it was not mentioned that CommandResult is being returned, it looks like it serves no purpose in this diagram, which would cause a developer to wonder why it was left in the diagram in the first place.

Would it be better to then have left the whole CommandResult entirely? That is debatable. The developers mentioned that the omission does not affect the central idea of the diagram, which is to understand the overall command execution sequence. But a key feature of Sequence Diagrams is that it also allows the developers to show how the execution interacts with other entities in the sequence, not just the sequence itself. This comes from the textbook:

image.png

And CommandResult is very much an important entity in the whole execution. Not only is it meant to be returned by StarCommand#execution, it is actually also returned from the LogicManager#execution call:

image.png

So it is not entirely a trivial entity either.

I would also ask that considering the diagram as a whole, there are many parts that are not "essential" to understanding the sequence of the execution (for example the saving to hard disk in the Storage component). Arguably, there are things that are missing (for example, a call to model.setStudent() is what changes the count of stars in the first place. From the code snippet of the StarCommand above, updateFilteredPersonList() on line 68 isn't even part of the Star changing logic, it merely resets the UI).

Hence, I feel that mentioning what is being returned is at least as important as other aspects of the diagram that have been left in. In particular, it explains why CommandResult even needs to be left in the diagram. Since this omission has an affect on the understanding of the diagram, I don't believe it can be chalked up to a cosmetic issue, and should not be VeryLow. It should minimally be a Low severity bug.