Skip to content

[CS2113T-T10-4] MindMyMoney#10

Open
GlendonNotGlen wants to merge 700 commits into
nus-cs2113-AY2122S2:masterfrom
AY2122S2-CS2113T-T10-4:master
Open

[CS2113T-T10-4] MindMyMoney#10
GlendonNotGlen wants to merge 700 commits into
nus-cs2113-AY2122S2:masterfrom
AY2122S2-CS2113T-T10-4:master

Conversation

@GlendonNotGlen

Copy link
Copy Markdown

MindMyMoney is a Command-Line application that allows users to track their spending

@Musfirahe0556596 Musfirahe0556596 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Great diagrams drawn! Some issues to consider!

Comment thread docs/DeveloperGuide.md Outdated
#### Current Implementation
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.
![add_command_sequence_diagram](images/Add_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps there should be an activation bar on the call to the AddCommandInputTest#test(...) method as shown in the screenshot?:
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the return values should include the data type e.g. category: String

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This applies to all the return value

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to enter the objectName:Datatype format for the return types?

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +114 to +116
4. During the execution, `Addcommand.execute` will parse through user input to obtain the `EXPENDITURE`, `CATEGORY`,
`DESCRIPTION`, `AMOUNT` and `TIME` fields.
5. The `Addcommand` object instantiates a new `Expenditure` object with the aforementioned 5 fields and adds them

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Perhaps there should be another line to describe what the AddCommandInputTest#test(...) method does?

Comment thread docs/DeveloperGuide.md Outdated
To enable users to view their finances in a more meaningfully, MindMyMoney does calculations to present financial data
that is actionable for the users.

![calculate_command_sequence_diagram](images/gif_loading.gif)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unsure if the gif is supposed to be an indication of 'to be updated' as here it states that the picture is supposed to be a sequence diagram, but no sequence diagram is shown xD.

Comment thread docs/DeveloperGuide.md Outdated
### Command component
The source can be found in [command](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/command)

![command_diagram](images/command_diagram.png)

@Musfirahe0556596 Musfirahe0556596 Mar 31, 2022

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This diagram is understandable as it shows the different commands available but perhaps an improvement that could be done is to add in the abstract method execute() also to show that all command types have this method in their own classes?

Comment thread docs/DeveloperGuide.md Outdated
The Sequence Diagram below shows an example of how the components interact with each other for the scenario
where the user issues the command`add shoes 100`.

![sequence_diagram](images/sequence_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Excellent diagram that shows a high-level overview of how the application works. Colouring to differentiate the different components are also good!

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For the sequence diagram, shouldn't the user go to :MMM with thecommand add shoes before the :Ui?
The sequence feels a bit off here

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think they are trying to illustrate that the user enters the command through :UI, which is then sent to :MMM to handle, which will then parse and do the rest of the sequence.

Comment thread docs/DeveloperGuide.md Outdated
<br/>

### UI component
The source code can be found in [Ui.java](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/Ui.java)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I like how you guys include the link for source code in the DG

Comment thread docs/DeveloperGuide.md Outdated

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}
### Architecture
![architecture diagram](images/architecture_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The architecture diagram does not have to connection between Parser and Commands, while the command diagram indicates otherwise.
image
image

Comment thread docs/DeveloperGuide.md Outdated
### Command component
The source can be found in [command](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/command)

![command_diagram](images/command_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The activation bar in the sequence diagram for the Command:AddCommand is too long
image

Comment thread docs/DeveloperGuide.md Outdated
To enable users to view their finances in a more meaningfully, MindMyMoney does calculations to present financial data
that is actionable for the users.

![calculate_command_sequence_diagram](images/gif_loading.gif)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I do not know why you guys put loading gif here but I think it should be a sequence diagram
image

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I thought it was intended but just curious why haha

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also curious about the loading gif! Is it a picture that was not rendered?

Comment thread docs/DeveloperGuide.md Outdated
![list_command_sequence_diagram](images/List_Command_Sequence_Diagram.png)

## Product scope
### Target user profile

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no information in the product scope, non-functional requirement, glossary and instruction for manual testing sections.
image
image

@wraineflores wraineflores left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DG peer review

Comment thread docs/DeveloperGuide.md Outdated
@@ -4,10 +4,150 @@

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no overview summary of what the software is all about

Comment thread docs/DeveloperGuide.md Outdated
<br/>

### UI component
The source code can be found in [Ui.java](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/Ui.java)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It's good that there's a source code hyperlink to the Github website

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +55 to +58
![parser_diagram](images/parser_diagram.png)
<br/> Fig 4 - Parser Diagram

The Parser component consists of a`Parser`, `Functions`, `ExpenditureList` and `Expenditure`class

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The labeling of figures may be better if placed above the diagram itself.
Why is the Function class separate from the Parser Class?

Comment thread docs/DeveloperGuide.md Outdated
![list_command_sequence_diagram](images/List_Command_Sequence_Diagram.png)

## Product scope
### Target user profile

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Target User is unknown so it is unclear who this software is for

Comment thread docs/DeveloperGuide.md Outdated
To enable users to view their finances in a more meaningfully, MindMyMoney does calculations to present financial data
that is actionable for the users.

![calculate_command_sequence_diagram](images/gif_loading.gif)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Calculate command sequence diagram is not loading maybe because it is a .gif instead of a .png image
Tutorial Week 11 Screenshot 5b

Comment thread docs/DeveloperGuide.md Outdated
Comment on lines +29 to +32
![sequence_diagram](images/sequence_diagram.png)
<br/> Fig 2 - Sequence Diagram

The sections below give more details for each component.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

There is no termination, loop, opt, etc. in the sequence diagram
Tutorial Week 11 Screenshot 6b

@ibrahimisramos ibrahimisramos left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good job team!

Comment thread docs/DeveloperGuide.md Outdated
The Sequence Diagram below shows an example of how the components interact with each other for the scenario
where the user issues the command`add shoes 100`.

![sequence_diagram](images/sequence_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

For the sequence diagram, shouldn't the user go to :MMM with thecommand add shoes before the :Ui?
The sequence feels a bit off here

Comment thread docs/DeveloperGuide.md
The source can be found in [`command`](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/command)

![command_diagram](images/command_diagram.png)
Fig 5 - Command Class Diagram

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to centralise the "figure 5". I took a while to find it

Comment thread docs/DeveloperGuide.md Outdated
#### Current Implementation
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.
![add_command_sequence_diagram](images/Add_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

the return values should include the data type e.g. category: String

Comment thread docs/DeveloperGuide.md Outdated
#### Current Implementation
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.
![add_command_sequence_diagram](images/Add_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This applies to all the return value

Comment thread docs/DeveloperGuide.md Outdated
To enable users to view their finances in a more meaningfully, MindMyMoney does calculations to present financial data
that is actionable for the users.

![calculate_command_sequence_diagram](images/gif_loading.gif)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I thought it was intended but just curious why haha

Comment thread docs/DeveloperGuide.md Outdated
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.

![add_expenditure_command_sequence_diagram](images/Add_Expenditure_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

for the print added expenditure, should it be calling the print()/equivilent print method?

Comment thread docs/DeveloperGuide.md Outdated
|Version| As a ... | I want to ... | So that I can ...|
|--------|----------|---------------|------------------|
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Might want to add more of the user stories, feels like the app has very limited features

@FTang21 FTang21 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall really good DeveloperGuide! Good job

Comment thread docs/DeveloperGuide.md Outdated

The sequence diagram below shows the interactions of when an `AddCommand` is parsed.

![add_command_sequence_diagram](images/Add_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screen Shot 2022-04-01 at 10 22 27 AM

Shouldn't the return arrow be outside of the alt frame?

Comment thread docs/DeveloperGuide.md Outdated
### Command component
The source can be found in [`command`](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/command)

![command_diagram](images/command_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screen Shot 2022-04-01 at 10 25 57 AM

Should this the lines from Parse to the Commands be dashed? I'm not sure if this a dependency as Parser is creating Commands

Comment thread docs/DeveloperGuide.md Outdated
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.

![add_expenditure_command_sequence_diagram](images/Add_Expenditure_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Screen Shot 2022-04-01 at 10 32 39 AM

I believe your `print added expenditure` self call is missing the activation bar. Similarly, in your addCreditCard() sequence diagram you seem to be missing the `print added credit card` activation bar.

Comment thread docs/DeveloperGuide.md Outdated
|Version| As a ... | I want to ... | So that I can ...|
|--------|----------|---------------|------------------|
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application|
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list|

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Like ibrahimisramos stated, adding more user stories would be good. It would also tell others what features you focus based on the user stories you looked at or plan to look at.

Comment thread docs/DeveloperGuide.md Outdated
The Sequence Diagram below shows an example of how the components interact with each other for the scenario
where the user issues the command`add shoes 100`.

![sequence_diagram](images/sequence_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

No critique here, just a really nice diagram sequence that details the architecture of your program!

@mathanmahe mathanmahe left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Looks good. It's a very informative developer guide with a few areas for improvement,

Comment thread docs/DeveloperGuide.md Outdated
The Sequence Diagram below shows an example of how the components interact with each other for the scenario
where the user issues the command`add shoes 100`.

![sequence_diagram](images/sequence_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think they are trying to illustrate that the user enters the command through :UI, which is then sent to :MMM to handle, which will then parse and do the rest of the sequence.

Comment thread docs/DeveloperGuide.md Outdated
The Sequence Diagram below shows an example of how the components interact with each other for the scenario
where the user issues the command`add shoes 100`.

![sequence_diagram](images/sequence_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good use of color schemes to make the sequence and architecture diagrams more visually appealing and easy to follow through!

Comment thread docs/DeveloperGuide.md Outdated
### Parser component
The source code can be found in [`Parser.java`](https://github.com/AY2122S2-CS2113T-T10-4/tp/blob/master/src/main/java/seedu/mindmymoney/Parser.java)

![parser_diagram](images/parser_diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to clarify this diagram? It is a bit confusing because a package named Parser is used. But the code architecture does not have a package named parser. Would it be possible to rename it to a more suitable name like ExpenditureTracker? Because it houses not only parser, but also the other aspects like the ExpenditureList, Functions as well as Parser

Comment thread docs/DeveloperGuide.md Outdated
#### Current Implementation
The sequence diagram below shows the interactions of different subcomponents of the system when adding an expenditure
to the list.
![add_command_sequence_diagram](images/Add_Command_Sequence_Diagram.png)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to enter the objectName:Datatype format for the return types?

Comment thread docs/DeveloperGuide.md
**Value proposition**
Manage finances containing multiple payment methods faster than a typical mouse/GUI driven app.

### User Stories

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Would it be possible to add more user stories that could cover what kind of users the features are intended for?

Comment thread docs/DeveloperGuide.md Outdated
To enable users to view their finances in a more meaningfully, MindMyMoney does calculations to present financial data
that is actionable for the users.

![calculate_command_sequence_diagram](images/gif_loading.gif)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Also curious about the loading gif! Is it a picture that was not rendered?

Comment thread docs/DeveloperGuide.md
* Mainstream OS: Windows, Linux, Unix, OS-X

## Instructions for manual testing

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Kindly do add instructions for manual testing of the features so a new developer can understand how to test these features. You can include positive and negative test cases, expected outcomes.

limjierui and others added 30 commits April 11, 2022 22:12
# Conflicts:
#	docs/images/AddCreditCardSequenceDiagram.png
#	docs/images/AddExpenditureSequenceDiagram.png
Standardise sequence and class diag format
Add validators for reading save file
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.