owx0130 / pe

0 stars 0 forks source link

almost illegible diagram in ArchiveCommands #24

Open owx0130 opened 3 months ago

owx0130 commented 3 months ago

image.png

activation bars are not correctly closed off, arrows pointing to the middle of the activation bars, no return arrow to the user. No : or <<class>> label used in StudentList to describe whether it is an object or that its class-level methods are being called

same reason as issue #22 why I gave this severity high

nus-se-script commented 2 months ago

Team's Response

*Duplicate of #781, as the only valid portion of this bug is about the activation bar being cut off incorrectly as per following explanation:

Overall, the purpose of this diagram is to show the flow of the function calls and we believe this flow is reflected clearly with all the function calls specific to the ArchiveCommands component Moreover, the sequence diagram still matches the earlier description and following explanation of the ArchiveCommands component. The poorly drawn activation bars or omitted details should not detract from the reader's understanding of the ArchiveCommands component at all, let alone the usage of the whole DG.

The proposed claim that this diagram is illegible and severity is high might be a little too harsh in our opinion, since it does not render the ArchiveCommands component "unusable" by the severity definitions.

Hence, we propose a severity of low instead since this should only cause minor inconvenience localised within this small part (the diagram) of the ArchiveCommands component.

The 'Original' Bug

[The team marked this bug as a duplicate of the following bug]

Activation bars cut off

Note from the teaching team: This bug was reported during the Part II (Evaluating Documents) stage of the PE. You may reject this bug if it is not related to the quality of documentation.


image.png

As can be seen from the image, the activation bars are cut off at the top and bottom. In fact this is prevalent across several other DGs. This is a signficant bug as it will prevent readers from understanding when method is called.

Hence, I am assigning this a severity of medium.


[original: nus-cs2113-AY2324S2/pe-interim#716] [original labels: severity.Medium type.DocumentationBug]

Their Response to the 'Original' Bug

[This is the team's response to the above 'original' bug]

We believe this is definitely a valid error . However, this should not hinder the understanding of the sequence diagram, let alone the whole archive component or entire DG itself.

The purpose of this diagram is to show the flow of the function calls and we believe this flow is reflected clearly, despite the inaccurate activation bars. In the DG above the diagram, it is mentioned the diagram shows the sequence of function calls when archive command is activated by the user, hence it should be pretty obvious from the diagram starting from when the user inputs "archive".

Hence, we propose a severity of low instead as it does not seem significant and this bug is unlikely to affect comprehension of the ArchiveCommands component at all, let alone the usage of the whole DG.

image.png

Items for the Tester to Verify

:question: Issue duplicate status

Team chose to mark this issue as a duplicate of another issue (as explained in the Team's response above)

Reason for disagreement: while i agree that there is duplication with regards to the activation bars being cut off, there is also another valid issue raised which the team rejected, which is related to omitting the : or <<class>> labels for StudentList.


## :question: Issue severity Team chose [`severity.Low`] Originally [`severity.High`] - [x] I disagree **Reason for disagreement:** the team mentioned that the `:` or `<>` labels were omitted for clarity due to both static and non-static methods being called, but this is not in line with the CS2113 sequence diagram drawing guidelines, as shown below: ![image.png](https://raw.githubusercontent.com/owx0130/pe/main/files/be08c8bc-254a-4a25-a32d-7c4d4d92176d.png) the diagram should have included another participant for `StudentList` with the `<>` label for the static `getStudent` method call, while creating two other participants of class `StudentList` with object names `masterList` and `archiveList` to make the non-static `remove` and `add` calls respectively. by writing the non-static method calls as `masterList.remove` and `archiveList.remove`, the reader is left confused as these objects were not explicitly created in the sequence diagram. additionally, the user does not know whether `getStudent` is a class level method or not due to the mislabeling of `StudentList`, creating significant inconvenience from choosing to omit the `:` or `<>` labels. the incorrect activation bars also misrepresent the duration of the method calls in the sequence diagram, which may confuse the reader about the interactions between the different objects in the sequence diagram. for example, under `ArchiveCommands`, the user could have interpreted the `archiveStudent` method as ending before the `NAME` input was given by the user, or they could also interpret it as the `archiveStudent` method taking in the `NAME` input by the user. the team also claims that the activation bars are largely cosmetic in this diagram, but i conversely feel that they are imperative in this diagram for the user to truly grasp the sequence flow of the feature. this is due to the fact that the feature utilizes multi-step commands, which will definitely confuse the user if activation bars were omitted/done incorrectly, as described in the example above. as a result, developers reading this will find it difficult to recreate the functionality depicted by this sequence diagram, as they are left confused about many aspects in the diagram. thus, i feel that the bug should be maintained at a severity high level due to the degree of inconvenience it causes the user.