Piyotato / pe

0 stars 0 forks source link

Unnecessary activity diagrams for index parsing in DG #15

Open Piyotato opened 1 year ago

Piyotato commented 1 year ago

image.png

image.png

There are multiple such diagrams which don't contribute much to the understanding of the code - in fact it might be even better to simply summarize them in words. It doesn't help me understand how the parser identifies a valid index either.

nus-pe-bot commented 1 year ago

Team's Response

Multiple diagrams are necessary and does not confuse the user. It may not contribute to give extra insights to everyone but it helps any developer visualise the process of the code. This cannot be considered a bug because a bug would mean that these diagrams had a negative impact on the reader. Furthermore, these diagrams are not placed together in one whole section, but are rather separated according to the command that it is describing. Thus, while the tester may not find it helpful, this wouldn't cause extra confusion to other readers.

Items for the Tester to Verify

:question: Issue response

Team chose [response.Rejected]

Reason for disagreement: I still don't see how these diagrams are necessary. This is literally just a simple branching - is it not obvious just from looking at code? Here are some words you can use to replace the diagram:

Upon receiving a command, the parser checks whether the given index is valid. If not, it throws an error. Otherwise, it proceeds with the execution.

Not only is this easier to read, but does not require the user to try interpret a diagram. I think any unnecessary diagram can be seen as a negative impact to the user, because they need to spend precious time deciphering it, even though reading a textual representation of it would just be faster. You are adding an activity diagram just for the sake of it. This also adds unnecessary length to the DG, making it harder to navigate for the user.

However, I could see an activity diagram being useful if it explains how the parser figures out whether a given index is valid.