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

[CS2113-T12-2] Big-PP #23

Open
wants to merge 615 commits into
base: master
Choose a base branch
from

Conversation

Magmanat
Copy link

@Magmanat Magmanat commented Mar 2, 2023

This is a command-line tool designed to help you pick computer parts based on your needs and budget.

kyrixn added a commit to kyrixn/tp that referenced this pull request Mar 13, 2023
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
Copy link

@SpawnageLoong SpawnageLoong left a comment

Choose a reason for hiding this comment

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

Overall good explanation, but class diagrams tend to be excessively detailed to the point that they cannot be read without zooming in.

docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
Copy link

@kairuler kairuler left a comment

Choose a reason for hiding this comment

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

Overall really nice DG :) I liked how some of the classes use a Initialization, Functionality, Behavior structure

docs/DeveloperGuide.md Show resolved Hide resolved
![Viewer Add Command Sequential Diagram](uml-pictures/ViewerAddCommand.png)
### PC Class Architecture

![PC Class Diagram](uml-pictures/PcDiagram.png)

Choose a reason for hiding this comment

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

image
image

In the DG, the Component class is said to be an abstract class but in the diagram, it isn't shown as an abstract class. If I didn't misunderstand the diagram, perhaps this is a mistake?

docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Show resolved Hide resolved
docs/DeveloperGuide.md Outdated Show resolved Hide resolved

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
### UI class

Choose a reason for hiding this comment

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

Maybe a multiplicity needs to be added between UI and UIState as well.

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.