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-T10-4] ExpenseTracker #10

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

Conversation

kq2003
Copy link

@kq2003 kq2003 commented Sep 19, 2024

Expense tracker help those who had a hard time tracking their own expenses daily. It is optimized since it saved the inputs from the user and output them efficiently.

private Map<Category, Budget> budgets = new HashMap<>();
private boolean isautoResetEnabled;
private int lastResetMonth;

Choose a reason for hiding this comment

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

Consider separating the class for better readability and maintainability. Having different classes for Expense, Category, and Budget would be better.

break;
}
}
if (existingCategory == null) {

Choose a reason for hiding this comment

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

The logic to find an existing category is repeated in multiple methods (addExpense, setBudgetLimit). Consider refactoring this into a private helper method to reduce redundancy.

double amount = 0;
String category = null;


Choose a reason for hiding this comment

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

Consider to add a check that ensures amounts are not negative in addExpense and setBudgetLimit.

@@ -0,0 +1,149 @@
package seedu.duke;

Choose a reason for hiding this comment

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

Consider adding more UTs.

#### delete-expense
![Delete Expense Sequence Diagram](diagrams/DeleteExpense.png)

#### tag-expense

Choose a reason for hiding this comment

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

No comments here.

![Add Expense Sequence Diagram](diagrams/AddExpense.png)

#### add-category
![Add Category Sequence Diagram](diagrams/AddCategory.png)

Choose a reason for hiding this comment

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

The return method should be dashed here.


## Design & Implementation
### Sequence Diagrams
#### add-expense

Choose a reason for hiding this comment

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

image
Calling them themselves here is missing the activation bar.

#### ExpenseTracker class
![ExpenseTracker Class Diagram](diagrams/ExpenseTrackerclass.png)

#### Duke class

Choose a reason for hiding this comment

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

image

You should use the open arrow here.

Copy link

@Lucky-Yuan00 Lucky-Yuan00 left a comment

Choose a reason for hiding this comment

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

A total of four questions were found and submitted.

Copy link

@crystal-bys crystal-bys 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 think it is pretty well done, just need to check formatting. Great job!

Comment on lines 109 to 118
<!-- @@author glenda-1506 -->
| Version | As a... | I want to... | So that I can... |
|---------|---------|--------------|------------------|
| v1.0 | Budget-conscious user | Quickly log an expense using a typed command (e.g., add 50 groceries) | track my spending with easy input |
| v1.0 | Budget-conscious user | View my budget for all categories | see how much I could spend |
| v1.0 | Budget-conscious user | Set a monthly reset for my budget tracking | start each month fresh with my budgeting |
| v1.0 | Budget planner | View all my expenses | monitor what I have been spending on |
| v1.0 | Frequent user | Set a budget limit for each category (e.g., set budget 200 groceries) | limit my spending according to categories |
| v1.0 | Frequent user | Delete an expense entry (e.g., delete 5) | quickly correct mistakes |
| v1.0 | Frequent user | Categorize expenses (e.g., add category food) | customize my expense tracking to better manage my budget |

Choose a reason for hiding this comment

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

Sorry, this is difficult to view in the html site. I have seen the User Stories here and I think it works, but you may want to check on the formatting.
image

Comment on lines 121 to 129
<!-- @@author mayfairmi6 -->
| ID | Requirement | Description | Rationale |
|-----|------------------|------------------------------------------------------|-------------------------------------------------------|
| 1 | Responsiveness | The system should respond to user commands within 2 seconds. | Ensures efficient interaction and enhances user satisfaction. |
| 2 | Data Integrity | The system must maintain accurate tracking and updating of financial entries. | Prevents discrepancies in financial reporting, ensuring reliability. |
| 3 | User Error Handling | The system should provide clear error messages and support easy correction of user inputs. | Facilitates management of entries and reduces user frustration. |
| 4 | Customizability | Users should be able to easily add and modify expense categories. | Allows users to tailor the system to their specific needs. |
| 5 | Automated Tasks | Support automated budget resets at the start of each month. | Minimizes user effort in maintaining accurate monthly tracking. |
| 6 | Accessibility | The chat interface should be simple and intuitive. | Ensures that all users can effectively interact with the system without extensive training. |

Choose a reason for hiding this comment

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

Similarly, the formatting of this is off.
image

![Add Expense Sequence Diagram](diagrams/AddExpense.png)

#### add-category
![Add Category Sequence Diagram](diagrams/AddCategory.png)

Choose a reason for hiding this comment

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

Is the activation bar for expense tracker supposed to end here? I believe the box should not go on like this.
image

![Add Category Sequence Diagram](diagrams/AddCategory.png)

#### delete-expense
![Delete Expense Sequence Diagram](diagrams/DeleteExpense.png)

Choose a reason for hiding this comment

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

Similarly for the other diagrams as well, for the arrows pointing left (for printing something) I believe they should be dotted as well (refer to the Week 10 notes)

Copy link

@jinzihan2002 jinzihan2002 left a comment

Choose a reason for hiding this comment

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

Good start to the DG! The sequence diagrams could perhaps be improved by extracting out segments using reference frames to improve clarity, adding activation bars for self-invocations and using dashed arrows for method returns.

ExpenseTracker -> User : No reset needed
end

deactivate ExpenseTracker

Choose a reason for hiding this comment

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

image

First activation bar for ExpenseTracker does not seem to be deactivated, for now it seems unclear when the first bar ends. Perhaps there should be a second deactivate ExpenseTracker here?

Choose a reason for hiding this comment

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

Perhaps segments of the sequence diagram could use ref frames to be shown as separate, smaller sequence diagrams? This could help reduce clutter of the sequence diagram.

Comment on lines 18 to 30
ExpenseTracker -> ExpenseTracker : Initialize expensesByCategory Map

loop Populate Expenses By Category
ExpenseTracker -> Expense : Get category for each expense
activate Expense
Expense --> ExpenseTracker : Return category
deactivate Expense

alt Category Exists
ExpenseTracker -> ExpenseTracker : Add expense to existing category list
else New Category
ExpenseTracker -> ExpenseTracker : Create new category list and add expense
end

Choose a reason for hiding this comment

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

image

Perhaps the Initialize expensesByCategory Map, Add expense to existing category list and Create new category list and add expense self-invocations should include activation bars for ExpenseTracker?

Choose a reason for hiding this comment

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

For the Expense class, as the Category association has already been shown with the line, perhaps it can be omitted from the attributes under Expense?

Copy link

@CT9ARyan CT9ARyan left a comment

Choose a reason for hiding this comment

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

LGTM 👍

![Tag Expense Sequence Diagram](diagrams/TagExpense.png)

#### set-budget
![Set Budget Sequence Diagram](diagrams/SetBudget.png)
Copy link

Choose a reason for hiding this comment

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

Screenshot 2024-11-02 003711

Should these return arrows here be dotted instead since they are returning from the method call?? I believe a solid return arrow means the current method is performing a callback to the caller. (i.e. calling another method from the caller)

#### view-expenses
![View Expenses Sequence Diagram](diagrams/ViewExpenses.png)

#### toggle-reset
Copy link

Choose a reason for hiding this comment

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

Screenshot 2024-11-02 005039

Shouldn't there be a return arrow from Duke to User for the input("toggle-reset") method?? The activation bar for that method just seems to end abruptly without returning anything.

![View Expenses Sequence Diagram](diagrams/ViewExpenses.png)

#### toggle-reset
![Toggle Auto Reset Sequence Diagram](diagrams/ToggleAutoReset.png)
Copy link

Choose a reason for hiding this comment

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

Screenshot 2024-11-02 003524

Would it be better if the frame tag is changed to opt instead since there is no "else" case??

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

## Design & Implementation
### Sequence Diagrams
Copy link

Choose a reason for hiding this comment

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

The following applies to ALL sequence diagrams:

According to the lecture notes, the UML notation for the sequence diagrams doesn't include the entity/object at the bottom of the lifeline.

To rectify this, maybe you could include hide footbox at the top of your .puml files of your sequence diagrams?? This should resolve this issue. :)

glenda-1506 and others added 24 commits November 10, 2024 15:36
Refactor Storage class to save all data
Refactor method of storing data to resolve corruption in data files
Reorganise file and categories them into different packages
Remove redundant files and lines and refactor UI
Refactor User Guide based on the updated code
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.

9 participants