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

[CS2113T-W12-4] Plan&Score #81

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

Conversation

elizabethcwt
Copy link

@elizabethcwt elizabethcwt commented Oct 7, 2020

Plan&Score is a Java command-line application that allows Primary 6 students to plan and track their classes, CCAs and test dates. This enables the students to remember their schedule so they can plan well in advance for their tests and score better.

yeapcl pushed a commit to yeapcl/tp that referenced this pull request Oct 25, 2020
Copy link

@jialerk jialerk 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 job, you seem to have a lot of diagrams for your develop guide :)


![quizwritestorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/QuizWriteStorage.png)

Figure 9. Sequence Diagram of the writing of data
Copy link

Choose a reason for hiding this comment

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

image
Maybe there might be an activation bar when you arrow back to your ControlManager


![quizreadstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/QuizReadStorage.png)

Figure 10. Sequence Diagram of the reading of data
Copy link

Choose a reason for hiding this comment

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

image
Maybe there might be an activation bar when you arrow back to your QuizListDecoder


![modelcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ModelComponent.png)

Figure 4. Class Diagram of the Model component
Copy link

Choose a reason for hiding this comment

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

image
Maybe you can extend the dotted arrow from ModelManager to Interactable
The EventManager Implementing from interface might be a dotted arrow instead

<br />

#### Storage component
![storagecomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/StorageComponent.png)
Copy link

Choose a reason for hiding this comment

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

image
Is StorageManager an interface? why are there dotted arrows pointing towards it

Each `StorageManager` reads in their respective data files through a `decoder` and writes to the same file through an `encoder`

##### Event Storage
![eventstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/EventStorageManager.png)
Copy link

Choose a reason for hiding this comment

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

image
Maybe your arrow for inheritance is pointed in the wrong direction
maybe you can add an association label for creates

Copy link

@hui444 hui444 left a comment

Choose a reason for hiding this comment

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

Good job on your DG! :)
However, do check your return arrows for the activation bar as many of your diagrams missed it.


It returns an `EventParameter` to `EventStorageManager`.

![eventreadstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/EventStorageReadSequence.png)
Copy link

Choose a reason for hiding this comment

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

image

Should there be a return arrow for the end of the activation bar in EventListDecoder (its self invocation)?
Consider adding an activation bar for EventStorageManager to keep the diagram consistent.


It returns a `String` to `EventStorageManager` for further writing.

![eventwritestorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/EventStorageSaveSequence.png)
Copy link

Choose a reason for hiding this comment

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

image

Should there be a return arrow for EventStorageManager?
Consider adding an activation bar for EventStorageManager to keep the diagram consistent.


Figure 9. Sequence Diagram of the writing of data

![quizreadstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/QuizReadStorage.png)
Copy link

Choose a reason for hiding this comment

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

image

Should decodeQuizFromString be written as decodeQuizFromString()?


#### Model component

![modelcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ModelComponent.png)
Copy link

Choose a reason for hiding this comment

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

image
Should the arrow pointing to the interface be a dotted line arrow?

Copy link

@TomLBZ TomLBZ left a comment

Choose a reason for hiding this comment

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

Shoudl this be a solid line instead?
image

Copy link

@TomLBZ TomLBZ left a comment

Choose a reason for hiding this comment

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

Some suggestions :)


#### Model component

![modelcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ModelComponent.png)
Copy link

Choose a reason for hiding this comment

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

Is this arrow too short? It is hard to see whether it is a solid line arrow or a dashed line arrow.

image


#### List event (<date> / today)

![listevent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ListEvent.png)
Copy link

Choose a reason for hiding this comment

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

Is "str" not descriptive?
image
Do you mean "printableEvents" or at least "string" ?


It returns an `EventParameter` to `EventStorageManager`.

![eventreadstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/EventStorageReadSequence.png)
Copy link

Choose a reason for hiding this comment

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

Maybe there can be an avtivation bar here.
image


#### List event (<date> / today)

![listevent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ListEvent.png)
Copy link

Choose a reason for hiding this comment

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

Maybe there can be a return arrow here.
image

Copy link

@pinfang pinfang left a comment

Choose a reason for hiding this comment

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

some sequence diagrams are missing activation bars


#### Model component

![modelcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/ModelComponent.png)
Copy link

Choose a reason for hiding this comment

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

image

can try to re-position the arrows so that the arrows are not covering the words or overlapping?


It returns an `EventParameter` to `EventStorageManager`.

![eventreadstorage](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/EventStorageReadSequence.png)
Copy link

Choose a reason for hiding this comment

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

image

does the part highlighted in yellow included in sequence diagram?
and the part circled in red, does the activation bar end?


### Help feature

![helpcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/Help.png)
Copy link

Choose a reason for hiding this comment

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

image

should there be a return arrow (the part i circled) ?
and for showToUser(), should it be inside the handleHelp()?


### Add feature

![addcomponent](https://raw.githubusercontent.com/AY2021S1-CS2113T-W12-4/tp/master/docs/diagram/Add.png)
Copy link

Choose a reason for hiding this comment

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

i could not find DataManager class in your code, i only see EventDataManager. is the naming of the class in the diagram correct?

AndreWongZH and others added 24 commits November 8, 2020 00:31
Update images, github link and PPP link
…into branch-update-dg

# Conflicts:
#	docs/AboutUs.md
Update developer guide UML diagrams
Update User Guide (Handle indentation of command 14)
…ception

Branch storage corrupted exception
…into Update-UserGuide

� Conflicts:
�	docs/UserGuide.md
Added more images and updated the userguide
Aliciaho and others added 30 commits November 9, 2020 20:44
Refine ppp & Fixed UG hyperlink
Update PPP, UG and DG authorship
…into branch-PPP

# Conflicts:
#	docs/DeveloperGuide.md
…into branch-PPP

# Conflicts:
#	docs/DeveloperGuide.md
#	docs/UserGuide.md
Add in authorship for dg and ug
…into update-UserStories

� Conflicts:
�	docs/DeveloperGuide.md
Update factory reset message
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.

9 participants