[CS2113-W12-4] FinTrack#5
Conversation
duckyfuz
force-pushed
the
master
branch
2 times, most recently
from
297a1f5 to
b9612c8
Compare
…an2004-AboutUs Modify aboutus.md to include Daniel Kwan
syCHEN1645
left a comment
syCHEN1645
left a comment
There was a problem hiding this comment.
Overall good job! Your code is mostly of good standard and quality. I see there are many assertion messages which look like errors or warnings for the user, so just a reminder that assertions cannot replace exceptions in error handling (please refer to the course website for why) in case some of you might have got it wrong.
| /** | ||
| * Main entry-point for the FinTrack application. | ||
| */ | ||
| public static void main(String[] args) { |
There was a problem hiding this comment.
The main method seems a bit long. Maybe more abstraction here?
| return 0.0; | ||
| } | ||
|
|
||
| return getExpensesView().stream() |
| public double getTotalExpense() { | ||
| double sum = 0; | ||
| for (Expense expense : expenses) { | ||
| assert expense != null : "Expense should not be null."; |
There was a problem hiding this comment.
Hope you are not using assertion to handle errors, as it is only meant to help developers verify the code.
| amount = Double.parseDouble(amountStr); | ||
| } catch (NumberFormatException e) { | ||
| LOGGER.log(Level.WARNING, "Invalid amount format: {0}.", amountStr); | ||
| throw new IllegalArgumentException("Amount must be a valid number."); |
There was a problem hiding this comment.
Good error handling! Maybe can organise the error messages into string constants, especially if they are reused somewhere else?
| */ | ||
| public class ExpenseList extends ArrayList<Expense> { | ||
|
|
||
| /** Comparator that orders expenses in reverse chronological order (newest first). */ |
There was a problem hiding this comment.
Please follow the same format (leave out 1st and last lines) as other comments.
| return changed; | ||
| } | ||
|
|
||
| /* ========== Validation & Invariants ========== */ |
There was a problem hiding this comment.
This doesn't seem to be in standard format? Remeber to remove it if it is no longer needed.
…eloper-guide Add manual testing instructions in developer guide
Add PPP for duckyfuz
Add user stories to DeveloperGuide.md
…-and-debugging-1 Improve parsing resilience to whitespace, enforce des/ as last arg, add non-finite amount checks
Update PPP for duckyfuz
Update DG for v2.0, update des/ in sequence diagrams
jyx0615
left a comment
jyx0615
left a comment
There was a problem hiding this comment.
Overall, the DG looks great and includes many useful diagrams.
Just a quick reminder — don’t forget to add the : before the entities.
| #### Reading User Input | ||
| `waitForInput()` owns the blocking read from `System.in` via a shared `Scanner`. The method prints a consistent `> ` prompt, trims whitespace, and returns an empty string when the user simply presses enter. If the input stream is closed or the scanner encounters an illegal state, the method logs the failure (`SEVERE`) and returns `EXIT_COMMAND`; this sentinel gives the caller a deterministic way to trigger a graceful shutdown without duplicating exception handling logic. Unexpected runtime exceptions are rethrown after being logged so they can be surfaced during development. | ||
|
|
||
|  |
There was a problem hiding this comment.
It seems like that the entities in a sequence diagram are objects not the method calls
There was a problem hiding this comment.
Maybe there's no need to include system method calls in the sequence diagram.
|
|
||
| The internal logic for `parseAddExpense` is shown below in this sequence diagram: | ||
|
|
||
|  |
There was a problem hiding this comment.
The activation bar seems to be too long.
There was a problem hiding this comment.
Maybe use the same tool for all diagrams to keep the style consistent.
|
|
||
| `printListOfIncomes(...)` and `printListOfExpenses(...)` render collections supplied by the model. Both methods iterate defensively: each entry is validated inside the loop, and malformed records are skipped with a `WARNING` log instead of aborting the entire render. Both incomes and expenses are shown newest first to surface recent cash flows. Each row is wrapped with a 80-character horizontal divider to improve readability, and dates are standardised to `yyyy-MM-dd` via a local `DateTimeFormatter`. | ||
|
|
||
|  |
There was a problem hiding this comment.
Same problem as previous. It seems like that the entities in a sequence diagram are objects not the method calls.
| 5. When `list-budget` is called, `Ui` prints a list of the budgets by calling `printBudgets` which receives a printable version of all the budgets which we get from `getBudgetsView()`. | ||
|
|
||
| Below is the sequence diagram of an instance of `budget` and `add-expense`: | ||
|  |
There was a problem hiding this comment.
Just to clarify — does ok refer to a string or a variable here?
| Parser --> FinTrack: (category, amount) | ||
| FinTrack -> FM: setBudget(category, amount) | ||
| FM --> FinTrack: ok | ||
| FinTrack -> Ui: printBudgetSet(category, amount) |
There was a problem hiding this comment.
Is this format correct? Should there be a return arrow indicating a success/failure message right below this?
| FinTrack -> Parser: parseSetBudget(command) | ||
| Parser --> FinTrack: (category, amount) | ||
| FinTrack -> FM: setBudget(category, amount) | ||
| FM --> FinTrack: ok |
| else | ||
| Ui -> Ui: print "Here is a breakdown of your expense:" | ||
| loop for each (category, amount) in expenseByCategory | ||
| Ui -> Ui: percent = (amount / totalExpense) * 100.0 |
There was a problem hiding this comment.
Ideally, your Ui component should not handle any calculations. Try to have FinanceManager perform this calculation, as it seems to be in charge of all calculations.
| 5. `totalExpense` and `expenseByCategory` is then fed into a function in `Ui` called `printSummaryExpense` to print the summary. | ||
|
|
||
| Below is a sequence diagram to illustrate how summary-expense works: | ||
|  |
There was a problem hiding this comment.
This check for "expenseByCategory is empty" could be redundant as totalExpense is <= 0 when expenseByCategory is empty and also when expenseByCategory contains zero-amount responses.
Thus, you could replace it with a single check e.g. "alt (no expenses recorded)"
| 5. When `list-budget` is called, `Ui` prints a list of the budgets by calling `printBudgets` which receives a printable version of all the budgets which we get from `getBudgetsView()`. | ||
|
|
||
| Below is the sequence diagram of an instance of `budget` and `add-expense`: | ||
|  |
There was a problem hiding this comment.
What does "ok" mean or refer to? Please be more specific.
| 5. When `list-budget` is called, `Ui` prints a list of the budgets by calling `printBudgets` which receives a printable version of all the budgets which we get from `getBudgetsView()`. | ||
|
|
||
| Below is the sequence diagram of an instance of `budget` and `add-expense`: | ||
|  |
There was a problem hiding this comment.
Is this format correct? Could there be return arrows labelled successMessage signifying whether the command executed by the user was successful or not?
| 5. `totalExpense` and `expenseByCategory` is then fed into a function in `Ui` called `printSummaryExpense` to print the summary. | ||
|
|
||
| Below is a sequence diagram to illustrate how summary-expense works: | ||
|  |
There was a problem hiding this comment.
Ideally, Ui should not perform this calculation as it should strictly receive data and display it. Perhaps consider having FinanceManager handle this as it is in charge of calculations.
|
|
||
| The following is an example class diagram that encapsulates the role of `FinTrack` and other modules `FinanceManager`, `Ui` and `Parser` when running `add-expense`: | ||
|
|
||
|  |
There was a problem hiding this comment.
Remove nonstandard << static >> and class icon from class diagram
| #### Reading User Input | ||
| `waitForInput()` owns the blocking read from `System.in` via a shared `Scanner`. The method prints a consistent `> ` prompt, trims whitespace, and returns an empty string when the user simply presses enter. If the input stream is closed or the scanner encounters an illegal state, the method logs the failure (`SEVERE`) and returns `EXIT_COMMAND`; this sentinel gives the caller a deterministic way to trigger a graceful shutdown without duplicating exception handling logic. Unexpected runtime exceptions are rethrown after being logged so they can be surfaced during development. | ||
|
|
||
|  |
There was a problem hiding this comment.
logger not necessary for sequence diagram
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
| This section describes the overall architecture and explores the core classes of FinTrack. | ||
|
|
||
| ### Architecture |
There was a problem hiding this comment.
well structured section that explains the separation between classes
| ## Non-Functional Requirements | ||
|
|
||
| {Give non-functional requirements} | ||
| ### Performance |
There was a problem hiding this comment.
good review of the project from usability point of view
|
Is the class meant to be labelled as static in <<>>? 
|
Maybe can keep the use of activation bar consistent? The activation bar for Ui disappears in [not empty]
|
Similar to previous comment, maybe can consider removing activation bars, if not add the activation bars for FinTrack and Ui?
|
Is the activation bar for parser too long? It starts before the method call
|
Fix visual bug on forced termination
Update puml diagrams and PPP
Update PPP for duckyfuz
…bugging Improve editability of persistence file, update UG, DG, PPP
Update PPP with ASCII guard, regex checks, and merged PRs
Update summary-expense sequence diagram
Update UG to clarify modifying budgets
… expenses as well as when deleting expenses
Fix DG visual bugs
Add exceeding or near budget checks when adding budget after existing…
Fix UG summary example
10 participants
FinTrack helps NUS Computer Engineering students to track their finances. It is optimised for CLI users so that expenses can be recorded faster by typing in commands.