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-W12-1] FinSec #142

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

Conversation

weigenie
Copy link

@weigenie weigenie commented Oct 8, 2019

No description provided.

FinSec *--> "1" UniqueClaimList
UniqueClaimList o--> "*" Claim
Claim *-left-> Id
Claim *-down-> Hidden

Choose a reason for hiding this comment

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

There are 3 pointers pointing to nothing :( Seems like a WIP but just take note

@@ -0,0 +1,69 @@
@startuml

Choose a reason for hiding this comment

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

There is an additional box appearing at the right side of the diagram

!include style.puml

box Model MODEL_COLOR_T1
participant ":ModelManager" as ModelManager MODEL_COLOR

Choose a reason for hiding this comment

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

This can be broken into smaller sequence diagrams as there are multiple actions within this diagram

@@ -0,0 +1,87 @@
@startuml

Choose a reason for hiding this comment

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

This can be broken into smaller sequence diagrams as there are multiple actions within this diagram

Copy link

@Kzrthikz Kzrthikz left a comment

Choose a reason for hiding this comment

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

The model component(section 1.4) class diagram seems messy. There are 3 floating composition arrows from claim. The arrow from contact pointing to parameters should state multiplicity clearly. Good job so far.


Step 2. The user executes `delete 5` command to delete the 5th person in the address book. The `delete` command calls `Model#commitAddressBook()`, causing the modified state of the address book after the `delete 5` command executes to be saved in the `addressBookStateList`, and the `currentStatePointer` is shifted to the newly inserted address book state.
.Add Claim Command Sequence Diagram
image::AddClaimSequenceDiagram.png[]

Choose a reason for hiding this comment

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

The activation bar for AddClaimCommandParser exceeds the return for cmd arrow.

*Step 6:* `AddIncomeCommand` would return a `CommandResult` to the `LogicManager` which would then be returned back to the user.

.Add Income Command Sequence Diagram
image::AddIncomeSequenceDiagram.png[]

Choose a reason for hiding this comment

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

This sequence is very similar to that of addClaim sequence diagram. Can consider removing it.

3) FinSecParser calls the CheckCommandParser::parse method. This instance of CheckCommandParser then takes in the rest of the string,
in this case: `1` +

* An `Index` instance is then created when the ParserUtil:parseIndex method is called. This method takes in the argument from the CheckCommandParser::parse method parameter

Choose a reason for hiding this comment

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

CheckCommandParser::parse should be highlighted


[source, java]

public CommandResult execute(Model model) throws CommandException {

Choose a reason for hiding this comment

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

Too length code.


[source, java]

public CommandResult execute(Model model) throws CommandException {
Copy link

Choose a reason for hiding this comment

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

Quite long. Is there a way to simplify it?

Copy link

@woon17 woon17 left a comment

Choose a reason for hiding this comment

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

DG title does not change.

Copy link

@CarbonGrid CarbonGrid left a comment

Choose a reason for hiding this comment

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

Claims in Model Class Diagram points to nothing.

Copy link

@Akimatsu98 Akimatsu98 left a comment

Choose a reason for hiding this comment

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

  1. For section 1.4 Model Component, model class diagram has overlapping arrows and the number 1 and * above both claim and contact classes are too close to each other.
  2. The rest is very detailed and well done. Do remember to remove the remaining old address book stuff.

Akimatsu98 pushed a commit to Akimatsu98/main that referenced this pull request Nov 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.