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-T12-1] ChChing #24

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

Conversation

Rayleigh47
Copy link

ChChing is a desktop app for tracking spending, and it uses a Command Line Interface (CLI) for managing finances. If you are someone who needs a simple interface to get a better hold of your finances, this app is for you!

vishnuvk47 pushed a commit to vishnuvk47/tp that referenced this pull request Mar 13, 2023
Copy link

@AkmalHanis AkmalHanis left a comment

Choose a reason for hiding this comment

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

The DG is incomplete. Non-Functional Requirements and Instructions for manual testing are not completed.
It may be better to focus on other classes rather than just all the Command classes. Storage, UI and Parser could be explained in the DG as well.

The main class in our program is the ```Record``` and ```RecordList``` abstract classes, in which ```Income```, ```Expense``` will inherit from ```Record``` and ```IncomeList``` and ```ExpenseList``` will inherit from ```RecordList```. Most commands will act on instances of the ```Income```, ```Expense```,```IncomeList``` and ```ExpenseList``` classes.

![Record Class](images/Record_RecordList_UML_class.png)

Choose a reason for hiding this comment

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

For this UML diagram:

  1. Abstract class for RecordList and Record is incorrect. Instead of << >> , it should be denoted as {RecordList} instead?
  2. The font size of the diagram is a little bit small, making it blurry and hard to read. Perhaps this could be changed to make it easier to read.

<br> ![edit income sequence diagram](images/EditIncomeCommand_sequence_diagram.png)

The edit expense command works in a similar way, with its sequence diagram as shown:
<br> ![edit expense sequence diagram](images/EditExpenseCommand_sequence_diagram.png)

Choose a reason for hiding this comment

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

This edit expense sequence diagram may not be necessary due to its similarity to the edit income sequence diagram shown above. On the other hand, this font is clearer and more comprehensive to read as compared to the above sequence diagrams.

is called, which iterates through the expenseList, ```expenses``` and prints the index as well as a completed string of
expenses in ```expenses```.

![ListExpenseCommand](images/ListExpenseCommand_Sequence_Diagram.png)

Choose a reason for hiding this comment

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

For this diagram:

  1. This image is named as a sequence diagram when it is actually a class diagram
  2. Other than showing ListExpenseCommand class inherits from Command class, this does not add much significance at this point of the dg
  3. Possible to move this class diagram up above

Choose a reason for hiding this comment

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

The execute portion of the class diagram is also incorrect. It should be something like execute( ... ): void.

Afterwards, the ```execute()``` method will call the ```printSelector()``` method from ```Selector```.
The ```printSelector()``` method will print all the available currencies in the selector hashmap.
The selected currencies will be marked with a ```[X]``` and the unselected currencies will be marked with a ```[ ]```.

Choose a reason for hiding this comment

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

Formatting in the DG is a little bit inconsistent. While ListExpenseCommand and EditIncomeCommand uses numbering to sequence the explanation, the other Commands are explained in one large chunk. It may be better to standardize the way of explaining.

### Record and RecordList

The main class in our program is the ```Record``` and ```RecordList``` abstract classes, in which ```Income```, ```Expense``` will inherit from ```Record``` and ```IncomeList``` and ```ExpenseList``` will inherit from ```RecordList```. Most commands will act on instances of the ```Income```, ```Expense```,```IncomeList``` and ```ExpenseList``` classes.

Choose a reason for hiding this comment

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

The notation for abstract class for Record and RecordList is wrong. It should be {Abstract} rather than << Abstract >>.

Overall, a good detailed class diagram provided

### Target and TargetStorage

The `Target` and `TargetStorage` class allows users to set a target for their ideal balance.

Choose a reason for hiding this comment

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

Good use of protected variables as seen from the # notation.

@@ -8,23 +8,128 @@

{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}

## Implementation

Choose a reason for hiding this comment

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

You could add the system architecture diagram in the next iteration of this DG to depict the high-level design of your software and make it more detailed.

| v2.0 | foreigner | view my entries in a different currency | read my incomes and expenses in a currency I am familiar with |
| v2.0 | user | set a target for my balance | improve my financial management |
| v2.0 | user | see the target i have set | remind myself of my target |
| v2.0 | user | reset my income/expense lists or both | have a fresh list |
## Non-Functional Requirements

Choose a reason for hiding this comment

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

Please add these details in the next iteration in your DG

Choose a reason for hiding this comment

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

Eg. Add sample test cases for commands

@L-K-Chng
Copy link

Overall, a detailed DG draft. Great job! Do note to correct the minor errors, add on to manual testing as well as provide a system architecture diagram. Hope to see more improvements in the next iteration of the DG.

### Target user profile

{Describe the target user profile}
Target users are people who are keen on improving their financial accountability
Copy link

Choose a reason for hiding this comment

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

Could this be elaborated on further? For example, what exactly is financial accountability?

Choose a reason for hiding this comment

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

To add on the above. Perhaps a brief description of how this product is able to help people to improve their financial accountability can be briefly stated?


### Value proposition

{Describe the value proposition: what problem does it solve?}
The value proposition of ChChing is its ability to track income and expenses on a daily basis.
Copy link

Choose a reason for hiding this comment

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

Could this be elaborated on further? How else does this application solve the problem better than other similar applications?

Comment on lines +119 to +132
| 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 |
| v1.0 | user | add new expense to the records | record all my expenses |
| v1.0 | user | add new income to the records | record all my incomes |
| v1.0 | user | view all the records | refer to them when I forgot my expenses and incomes |
| v1.0 | user | edit the records | modify/fix if the records is changed/wrong |
| v1.0 | user | know current balance | aware how much money do I have left |
| v2.0 | user | edit my existing entries | rectify or update any entries without having to enter another entry and delete an entry |
| v2.0 | user | find specific entries | refer to specific entries when necessary |
| v2.0 | foreigner | view my entries in a different currency | read my incomes and expenses in a currency I am familiar with |
| v2.0 | user | set a target for my balance | improve my financial management |
| v2.0 | user | see the target i have set | remind myself of my target |
| v2.0 | user | reset my income/expense lists or both | have a fresh list |
Copy link

Choose a reason for hiding this comment

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

Great user stories!! Your application accurately solves the problems listed.

The ```printSelector()``` method will print all the available currencies in the selector hashmap.
The selected currencies will be marked with a ```[X]``` and the unselected currencies will be marked with a ```[ ]```.

![SetCurrencyCommand_sequence_diagram.png](images%2FSetCurrencyCommand_sequence_diagram.png)
Copy link

Choose a reason for hiding this comment

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

To add on to the above, the formating of the sequence diagrams themselves are slightly inconsistent, in terms of the colour of the boxes of the instances, and the word font colour of the operations.

hyperbola-bear and others added 25 commits April 3, 2023 11:30
Update Thomas's Project Portfolio Page
Update runtest's expected output to allow Java CI to pass
Add Non-functional Requirements in Developer Guide
thomasjlalba and others added 30 commits April 9, 2023 17:22
Remove console handler for logging
Add page break to neaten make Developer Guide pdf compatible
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.

8 participants