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-1] FlashSpeed #40

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

Conversation

ncslzh
Copy link

@ncslzh ncslzh commented Feb 19, 2020

Copy link

@artlee06 artlee06 left a comment

Choose a reason for hiding this comment

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

Good Effort guys! Refer to the comments made for some suggestions on what we think could improve your Developer Guide. Also consider having a variety of diagrams instead of just sequence diagrams.

@@ -122,21 +122,22 @@ image::ModelClassDiagram.png[]
The `Model`,

Choose a reason for hiding this comment

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

Figure 7 is a pretty good diagram, but some things to take note. Consider the 2 arrows pointing to UniqueCardList, the two 1s initially confused us as being 11 so it might be better if you separate them a little more. Also, we would love it if the diagram was smaller. Cheers!


The following sequence diagram shows how the `flip` operation works.

image::FLipSequenceDiagram.png[]

Choose a reason for hiding this comment

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

This image does not render, perhaps you missed something? Fix it soon, okay? :)


The following sequence diagram shows how the `play` operation works.

image::PlaySequenceDiagram.png[]

Choose a reason for hiding this comment

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

This sequence diagram and the subsequent sequence diagrams refer to their returned Command sub objects as "e". We think it should correspond to the first alphabet of the name of the subclass.

Also the rightmost "e" under the Model box is a little ambiguous as what it refers to. Perhaps the GameManager instance. Regardless please reflect it on the next iteration of the diagram.

Also it is unclear as to where commandResult is instantiated. Consider showing where it is being in constructed.

For your consideration. Cheers!

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.

6 participants