woojiahao / pe

1 stars 0 forks source link

Inaccurate sequence diagram, missing model parameter #18

Open woojiahao opened 10 months ago

woojiahao commented 10 months ago

The sequence diagram under the Logic component of the DG does not use execute(model), instead it uses execute() which is not a valid method in the codebase given the following method signature for execute:

public abstract class Command {

    /**
     * Executes the command and returns the result message.
     *
     * @param model {@code Model} which the command should operate on.
     * @return feedback message of the operation result for display
     * @throws CommandException If an error occurs during command execution.
     */
    public abstract CommandResult execute(Model model) throws CommandException;

}

Screenshot 2023-11-17 at 17.16.42.png

This is also present in the other execute() calls in the sequence diagrams:

Screenshot 2023-11-17 at 17.24.22.png

nus-se-script commented 10 months ago

Team's Response

Thank you for pointing this out, however, we have intentionally omitted the arguments for execute in order to reduce clutter as we do not feel that it adds a lot to the diagram. This is acceptable according to the textbook as seen here:

image.png

Items for the Tester to Verify

:question: Issue response

Team chose [response.NotInScope]

Reason for disagreement: I think the team has misunderstood the section in the textbook about minimal notation. While it is possible to reduce the use of certain notation (i.e. return arrows and activation bars), they can only removed IF the omission does not result in ambiguities or loss of information (as seen in the screenshot sent).

However, when execute() is used, it is indicating that a method call of v.execute() is made. As mentioned in my initial report with the code snippet of the execute(Model) method signature, this method must receive a Model parameter. Therefore, by intentionally omitting the arguments of the execute method, it definitely creates ambiguities about the method that is being called since execute() without parameters does not exist. This, in turn, makes the sequence diagram inaccurate as it does not properly document the methods that are being called and can cause confusion in readers.

we do not feel that it adds a lot to the diagram

I strongly believe that accuracy is a major component of a sequence diagram, otherwise, there is no point in using diagrams in the first place. Hence, the model parameter absolutely adds value to the diagram.

If the team had wanted to omit the use of model, they should have followed the textbook's section they had cited and changed it to executes without indicating that it is a method call.