Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DG] Diagrams have low interpretability #770

Open
nus-pe-bot opened this issue Nov 16, 2024 · 1 comment
Open

[DG] Diagrams have low interpretability #770

nus-pe-bot opened this issue Nov 16, 2024 · 1 comment

Comments

@nus-pe-bot
Copy link

nus-pe-bot commented Nov 16, 2024

Architecture diagram: Arrows do not show the nature of the interaction between these components. The diagram presents unnecessary coupling (not all classes have to interact with UI directly, e.g. Storage - UI, and even if they are implemented that way, you can omit the less important interactions by deleting the arrow connecting them).

Class diagrams and Sequence diagrams are cluttered and hard to read to a serious extent. A lot of trivial details can be omitted or abstracted from both Class diagrams and Sequence diagrams for interpretability, e.g. private methods in some classes. Some diagrams can be removed altogether (e.g. help command Sequence Diagram, as its content should be inferred from the overall sequence diagram already).

Here is a non-exhaustive breakdown of some diagram flaws:

  • The TrackerData diagram can be broken into two parts.

image.png

  • Overall Sequence Diagram:

image.png

UI should be what stands between the user and the internal system. Not achieving this is a sign of poor SLAP (single layer of abstraction principle) and SRP (single responsibility principle). The diagram implies that the only use of UI is to print a welcome message.

  • For sequence diagrams, take the screenshot below as an example:

image.png

The diagram above does not exactly distinguish between Parser and InputParser class instances. The three calls to InputParser are similar in nature and thus don't have to be shown three times. TrackerData need not be included in this diagram as Storage can be abstracted away from here (you can dedicate a section to explaining how Storage components work in more detail). Format seems like a utility class and can also be omitted (e.g. put behind a 'black box').


[original: nus-cs2113-AY2425S1/pe-interim#766] [original labels: severity.Medium type.DocumentationBug]
@glenda-1506
Copy link
Contributor

Team's Response

The diagrams are detailed and accurately represent the system’s architecture and flow, effectively fulfilling their purpose without causing misinterpretation of functionality. While they may require some effort to analyze, they are logically arranged, supported by explanatory text, and do not hinder understanding for the intended technical audience. The issue seems to be more about a preference for simplification rather than a flaw in usability or accuracy. Since the diagrams are complete and do not obstruct system implementation or maintenance, we assess this issue as having low severity. Any future simplifications would serve as enhancements rather than necessary fixes.

Duplicate status (if any):

--

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants