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

[CS2103T-W17-2] Notably #29

Open
wants to merge 1,854 commits into
base: master
Choose a base branch
from

Conversation

kevinputera
Copy link

[TIP]
The `.puml` files used to create diagrams in this document can be found in the link:{repoURL}/docs/diagrams/[diagrams] folder.
Refer to the <<UsingPlantUml#, Using PlantUML guide>> to learn how to create and edit diagrams.

[[Design-Architecture]]
=== Architecture

.Architecture Diagram
image::ArchitectureDiagram.png[]

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Intuitive architecture diagram, however possibly a little too detailed. Consider only showing high-level details so it is clearer for developers.

.Structure of the Logic Component
image::LogicClassDiagram.png[]
.Architecture of Logic
image::LogicArchitectureDiagram.png[]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool diagram! However, some features of a class diagram are present (dotted arrows, aggregation). Since it is a architecture diagram, it is not clear what they mean. Again, aim to capture high-level details in architecture diagrams. Unless you meant to make it a class diagram, where you would need to add more details and clarity. Maybe referring to AB3 architecture diagrams will be helpful.

==== CorrectionEngine component

.Class Diagram of the CorrectionEngine Component
image::CorrectionEngineClassDiagram.png[]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The interactions between CorrectionEngine to CorrectionStatus and CorrectionResult are not explained and cannot be inferred from the diagram. Maybe some clarification would be useful for developers to understand this.


.Interactions Inside the Logic Component for the `delete 1` Command
[[Design-NotablyParser]]
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No info on the NotablyParser component, maybe a mislabelling of the section?

[TIP]
The `.puml` files used to create diagrams in this document can be found in the link:{repoURL}/docs/diagrams/[diagrams] folder.
Refer to the <<UsingPlantUml#, Using PlantUML guide>> to learn how to create and edit diagrams.

[[Design-Architecture]]
=== Architecture

.Architecture Diagram

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be good if the architecture diagram was used straight lines instead of curved ones, so that the diagram is less confusing.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The architecture diagram seems a bit confusing. Could draw clearer where the commons is coming from! Otherwise, the architecture diagram seems to have been refactored greatly, keep up the hard work!


.Structure of the UI Component
.Structure of the View Component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lines pointing from SuggestionsList, SideBarTree and BlockContent seem very congested and they overlap a lot, making it seem confusing. Can try to separate them!


.Interactions inside the StringCorrectionEngine component for the `correct("uncorrected")` call
image::StringCorrectionEngineSequenceDiagram.png[]

[[Design-Model]]
=== Model component

.Structure of the Model Component

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you intend for the relationship between BlockTree and BlockTreeItem to be composition?

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

Successfully merging this pull request may close these issues.

7 participants