chewjh1234 / pe

0 stars 0 forks source link

Unclear return arrows #21

Open chewjh1234 opened 10 months ago

chewjh1234 commented 10 months ago

While return arrows are optional, I am not sure from which object are you returning to :LogicManager

The return arrow should be drawn on different levels. Screenshot 2023-11-17 at 5.33.06 PM.png

nus-se-bot commented 10 months ago

Team's Response

Thank you for the observation.

The implementation of the AddTagCommand results in the creation of an AddTagCommand object and its CommandResult is returned back to the logic manager class exceuting this command. Although the DG aims to clarify this, we do realise that the return arrow should be drawn on different levels. However, given the fact that the sequence diagram represents the sequential flow of control and that once the CommandResult is created, control logically flows back to the LogicManager, as it is the class responsible for initiating the command execution process, these sequence of actions are clearly communicated; hence, avoiding inconveniences for fellow developers(low severity bugs require a minor inconvenience).

Hence, we accept this as a Documentation bug with Severity.VeryLow.

Items for the Tester to Verify

:question: Issue severity

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

Reason for disagreement: This is more than a cosmetic issue. In fact, I am confused.

Let me explain.

Let us look at what happens before the CommandResult object is returned to the LogicManager. Let's focus on the portion just before the return arrow in question.

Screenshot 2023-11-21 at 4.12.33 PM.png

According to the sequence diagram, the constructor for CommandResult, CommandResult(), is called.

But hold on. The constructor CommandResult() does not instantiate a new CommandResult object. (If you wished to show that the constructor CommandResult() instantiates a new CommandResult object, you would have stuck the name of the object to the activation bar).

Therefore, the sequence diagram implies that the success of the command is already pre-determined because the CommandResult object, s, has already existed even before you run the command addt. It seems like you are able to predict the correct command result message even before the user runs the addt command.

The confusion does not end there.

You have labelled the CommandResult object as s:

Screenshot 2023-11-21 at 4.22.48 PM.png

But s is never used anywhere else in the entire sequence diagram. So it makes me wonder what object is returned to LogicManager. When I looked at the return arrow, I saw this:

Screenshot 2023-11-21 at 4.25.32 PM.png

and CommandResult is a class. So instead of returning the object s, your diagram suggests that you return the class CommandResult instead.

And my confusion does not end there.

The use of return arrows in your sequence diagrams are is inconsistent. And when this long return arrow is drawn on the same level, I have to guess what object is being returned to LogicManager.

Is it s, the CommandResult object? But you didn't label s.

Is it the CommandResult class? But why return the class?

Or is an object returned from AddTagCommand instead? Or is an object returned from AddTagCommandParser instead? After all, these objects come from classes which share the same word, Command, as CommandResult.

I had to guess, and all these reasons impeded my ability to understand the diagram.

The diagram's credibility is severely hit as well. According to the course website:

Screenshot 2023-11-21 at 4.38.17 PM.png

Hence, severity.Low should be kept