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

[CS2113-W12-3] FinText #7

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

Conversation

hooami
Copy link

@hooami hooami commented Oct 4, 2023

FinText is a personal finance tracker to make it easy for users to track and manage their spending, and generate daily/weekly/monthly reports to break down how they spend (e.g. spending categories, whether they spend above their income, etc.)

@hshiah
Copy link

hshiah commented Nov 2, 2023

image

should be {abstract} instead of {{abstract}}

@hshiah
Copy link

hshiah commented Nov 2, 2023

image
Why here Ui gets the user input instead of the Main receive the user input directly ?

Copy link

@karishma-t karishma-t left a comment

Choose a reason for hiding this comment

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

Overall really neat! Good effort put into using visual and verbal representation!

Choose a reason for hiding this comment

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

Is this too complicated? Might there be a way to display the top half better?

Choose a reason for hiding this comment

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

Could Transaction possibly be complicated/harder to read?

All error handling is handled here and any errors/output would be passed to the `UI` component for printing and
formatting of the output.

| Command | Purpose |

Choose a reason for hiding this comment

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

Clear summary for command components! Perhaps some sample inputs could be shown to demonstrate usage?

Choose a reason for hiding this comment

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

Might there be a more intuitive/clearer manner of emphasising the goal, and displaying transaction?

Copy link

@aaronxujiachen aaronxujiachen left a comment

Choose a reason for hiding this comment

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

Overall I find the developer guide is very detailed and logical with sufficient images inserted. Good job and wish you all the best for the upcoming PE.

Comment on lines 9 to 17
### Architecture

The bulk of the app's work is done by the following three components:
- `UI`: The UI of the App.
- `Parser`: Formats the user's input
- `Command`: Command's logic and execution
- `Storage`: Storage of data of the App

![Architecture Diagram](./images/ArchitectureDiagram.png "Architecture Diagram")

Choose a reason for hiding this comment

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

Very clear illustration of the project architecture!

### Storage component
The `Storage` functionality is to load data from the storage files (`category-store.csv` , `expense-store.csv`, `goal-store.csv`, `income-store.csv`) into the application. It will also stores any data while the application is running.

![Storage Class Diagram](./images/cs2113-storage-class.png "Storage Class Diagram")

Choose a reason for hiding this comment

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

The line drawn from CsvReader to OpenCSV should be straight preferrably.

### Target user profile

{Describe the target user profile}
Users who prefer a CLI interface over a GUI and want to better manage their finances to gauge their financial health.

Choose a reason for hiding this comment

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

Perhaps can identify a more specific target group, e.g. SOC students.

| HelpCommand | Gives usage format information to the user |
| ListCommand | Lists all incoming/outgoing transactions |
| ExportCommand | Exports transactions data into CSV FIle. |
| RemoveTransactionCommand | Deletes a transaction |

Choose a reason for hiding this comment

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

Perhaps DeleteCommand is a better name since it is more concise and suits the purpose.

Copy link

@limyuhching limyuhching left a comment

Choose a reason for hiding this comment

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

Overall, the structure closely mimics AB3 and is very neat.


### UI component

![UI Sequence Diagram](./images/cs2113-ui-sequence.png "UI Sequence Diagram")

Choose a reason for hiding this comment

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

Sequence diagram gave a very clear overview of how the 4 main components interact with each other

Comment on lines 96 to 97
## Implementation

Choose a reason for hiding this comment

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

Do rmb to add this

Comment on lines +72 to +73
![Storage Class Diagram](./images/cs2113-storage-class.png "Storage Class Diagram")

Choose a reason for hiding this comment

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

Good use of class diagram to show association with openCSV

Comment on lines +50 to +51
### Command component

Choose a reason for hiding this comment

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

Neat table

Copy link

@wwweert123 wwweert123 left a comment

Choose a reason for hiding this comment

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

Overall, a very comprehensive DG especially the parts on the diagram. However you may want to omit certain private methods or attributes to make the diagram less complicated. Remember to fill up other parts of the DG as well


":UI" -> User: Gets user input
activate "User" #FFBBBB
User --> ":UI": "userInput: String"

Choose a reason for hiding this comment

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

Maybe you can remove the quotation marks around userInput: String and for other instances in your seq diagram
image

+ printGreeting(): String
+ printBye(): String
+ readUserInput(): String
+ close(): void

Choose a reason for hiding this comment

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

You might want to keep your diagrams simple and omit unimportant and private members
image

}
}

Ui --> "{{abstract}}\nOutputStream" :> uses

Choose a reason for hiding this comment

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

You might want to add the multiplicity of association to outputStream and Scanner (should 1?)

activate ":Parser" #FFBBBB
":Parser" --> ":Main": "p: Parser"

":Main" -> ":Parser": p.parse(userInput: String)

Choose a reason for hiding this comment

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

You might want to break down your complicated diagram here into subdiagrams. This can be done using ref frames
image

@@ -0,0 +1,17 @@
package seedu.duke.classes;

public class Category {

Choose a reason for hiding this comment

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

  1. Maybe can use an enum for this? Similar to recurrence.
    image

@hshiah
Copy link

hshiah commented Nov 2, 2023

May use enumration for category.
image

@hshiah
Copy link

hshiah commented Nov 2, 2023

image
May replace "command:Command" to Command

Copy link

@Cazh1 Cazh1 left a comment

Choose a reason for hiding this comment

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

Overall good job on this DG. It is very detailed with clear explanation of the workings behind the classes. All the best for the PE!

Copy link

Choose a reason for hiding this comment

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

image
Should main have an activation bar?

Copy link

Choose a reason for hiding this comment

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

image
Perhaps consider removing the private static Strings to improve readability.

Copy link

Choose a reason for hiding this comment

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

image
Perhaps you meant to write (hasGeneratedNextRecurrence:boolean)?

Copy link

Choose a reason for hiding this comment

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

image
I really like the way this is presented. The use of comment and folder really helps the reader visualize the operations behind the class.

Jonoans and others added 30 commits November 13, 2023 23:09
Edit message for SummaryCommand
Update DeveloperGuide.md
Update UserGuide and change Goal message
Update UserGuide and PPP
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.