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

[CS2103-W14-1] Courses Management #28

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

Conversation

h12567
Copy link

@h12567 h12567 commented Feb 18, 2020

No description provided.

Copy link

@BransonNg BransonNg left a comment

Choose a reason for hiding this comment

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

Overall, looks great! Nice enhancements to your DG and nice diagrams!!

However, we noticed a couple of things:

Figure 9

  1. there's a rogue textbox with "text" in it
  2. the getFactory does not seem to fit the format of a class diagram's method description
  3. Class diagram also shouldn't show a simplified flow
  4. AssignCommandBase's COMMAND_WORD should be a static attribute? (we're not too sure either based on just diagrams)
  5. Maybe the modifiers should be used consistently

Figure 10

  1. Relationships indicated between classes should be represented as either attributes or lines with the multiplicity (for e.g. between Progress and ID -> you should only choose one)

Figure 12

  1. There some arrows in the diagram that overlap. Those classes could perhaps be shifted to the left to reduce the overlap.

General

Diagram designs can be more consistent throughout the DG

tiendat1601 and others added 29 commits April 9, 2020 00:35
completed undo/redo for assign
getIndex return index of obj in the list in address book
EdgeManager support for opposite Command of delete
Fix bug with clear-all command
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