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

[F11-4] NUS Hangs #115

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

Conversation

Happytreat
Copy link

@jelneo
Copy link

jelneo commented Oct 7, 2018

Good effort in hitting v1.1 milestone.

Some general feedback:

  • Update this:
    image

  • Travis build status badge should point to the build status of your own repo, not se-edu/AB-4. Other badges should be updated to match your project or removed.

Feedback on About Us page:

  • You should have components assigned to members

Feedback for User Guide:

  • Add ‘coming in v2.0’ for features not yet implemented

Feedback for Developer Guide:

  • For edit a friend use case:
    image
    Shouldn't the highlighted numbering be 5a instead since the system does not check for the existence of the person after asking for user confirmation.
  • For create account use case:
    image
  1. By wrong login details, you mean invalid login details? Like invalid characters in username/ password?
  2. Was the account already created successfully? Seems like the extension of this use case is for another use case where an account was already created and the user enters the wrong login information.

@jelneo
Copy link

jelneo commented Oct 21, 2018

Good job on hitting milestone v1.2 @Happytreat @aspiringdevslog @souless94 @ZhiWei94 @nigelngyy

Some general feedback:

  • Melodies' face in the picture in the About Us page is a little too small.
  • please update the portion below:

image

Some feedback on dev guide:

  • 3.1. Timetable Feature
    The current implementation section should explain how the feature is implemented and why it is implemented that way. It should be written with the objective of explaining the implementation to a future developer. The example usage scenario for this section should be written with the focus of explaining the implementation of the methods or certain design choices. Hence, it should be used to show the behaviour of the method step-by-step.
    The images can be bigger.
    Reorganize and follow the format of the sequence diagram examples given in AB-4 i.e. text box should be formatted properly with transparent background, activation bars aligned properly. Also, Person object is missing an activation bar for the constructor.
    image

The explanation can be improved to justify your design choice above. Perhaps you can explain a bit more?

  • 3.2. Security Mechanism
    For the activity diagram, it would be good to have a portion about choosing the password for a more complete picture of the workflow.
    More elaboration needed on how the feature is implemented and why it is implemented that way.
    For design considerations section, ensure the formatting is consistent with the rest of your members.

  • 3.3. Group Feature
    Use a diagram e.g. sequence diagram to illustrate interactions between components or method etc. and explain your implementation

  • Overall comments: standardise the format of the guide. E.g.
    image
    VS
    image

@Happytreat
Copy link
Author

@jelneo Updated Developer Guide with new diagrams

@jelneo
Copy link

jelneo commented Nov 6, 2018

Apologies, I just saw your comment @Happytreat @ZhiWei94 @souless94 @aspiringdevslog @nigelngyy
Some things to note:
This should be changed to your project name:
image
In general, diagrams need to be big enough for the text inside it to be read easily.
Make sure you contextualize the whole dev guide to according to your product if you have not done so.
3.1 Timetable feature
Remove 'Condition' inside the triangle. The conditions should be in square braces: []. Be specific about which command is entered (see blue annotations):
image

For the image below:

  1. what is the instance u? If it is the AddTimetableCommand that is created, then it should be at.
  2. there should only be 1 method per arrow. Do not lump them together.
    also, it is advisable to make the image bigger such that the text can be easily read.
    image

3.2. Security Mechanism
The portion in red should be broken up into alternate paths. In this case, we should prompt for login before we allow the user to execute any commands. Indicate the Model component like how you indicated a Logic component with the colored box.
image

The problem with the below image is that the control is returned back to CreateCommandParser when CreateCommand is not done with objection construction (there's still addNewAccount(...) which is called after the return arrow). Hence you should fill up the activation bar as shown in blue and shift the return arrow down as shown in red. Indicate the Model component like how you indicated a Logic component with the colored box.
image

For the image bloew:

  1. the return arrow should be at the end of the activation bar
  2. same comment as the sequence diagram for Create Command
    image

3.3. Group Feature
For the image below:

  1. remove the yellow box
  2. Where is the start of the flow? Is it starting from Text UI or from other components?
  3. the start and end of an arrow should be touching an activation bar. Also, the end of the arrow should be below the start of the activation bar (see e.g. in blue)
  4. Arrow should be pointing to RegisterCommandParser. Arrowhead missing as well.
  5. Activation bar and return arrow are placed wrongly for object creation.
  6. Format the arrowhead properly.
  7. arrow should touch the activation bar.
  8. floating return arrow. the end of the arrow needs to be coming from some process
  9. missing lifelines after the activation bars for all the classes.
    image
  10. Indicate the Model component like how you indicated a Logic component with the colored box.
    Correct Sequence Diagram of RegisterCommand (Model component) according to the comments above too.
  11. Make the background of the text box transparent

For the image below:

  1. wrong notation for denoting the end of an activity diagram
  2. should there be a loop? Must the user enter the find command again if the result of the find command is returned or the user entered an invalid find command?
  3. Why do we need to find exit command?
  4. It would be good to be specific about what results are returned. for e.g. System returns list of users or System returns result of find command if it returns a list of objects from multiple categories.
    image

Happytreat and others added 30 commits November 12, 2018 20:30
DeveloperGuide: Update DG for User Stories
DG: Update Person Feature
Edit add_timetable description
Edit group part formatting error
This reverts commit 18b409c.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants