-
Notifications
You must be signed in to change notification settings - Fork 233
[CS2113-F12-1] FinTrek #37
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
base: master
Are you sure you want to change the base?
[CS2113-F12-1] FinTrek #37
Conversation
added addExpense and help prompt
nicholascxh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job!
docs/DeveloperGuide.md
Outdated
|
|
||
| * **General and Recurring Expenses list**: A general list would save all the general expenses created by the user, while recurring expenses list comprise of expenses that will be added at their respective stipulated dates. | ||
|
|
||
| * **Storage**: `DataHandler` will hanlde downloading and uploading both general and recurring expenses to `data.txt` file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handle*
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nested alt should have a success condition
docs/diagrams/class/overall.puml
Outdated
| @@ -0,0 +1,73 @@ | |||
| @startuml ArchitectureOverview | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You may need to disable the symbols for classes
| ExpenseOperation <|.. RecurringExpenseManager | ||
|
|
||
|
|
||
| @enduml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, very detailed!
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Objects should have a colon infront of the names
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think method name cannot be put inside sequence diagram as an object.
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unsure what is N and M
docs/DeveloperGuide.md
Outdated
| - Extendability: Easily extensible to support `/delete-recurring`. | ||
|
|
||
| ### List Expenses | ||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Difference in sequence diagram for this one, there is numberings here.
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similarly, I think there should not be method as part of headers.
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, all the sequence diagrams looks very comprehensive, nice!
docs/DeveloperGuide.md
Outdated
|
|
||
| * **Storage**: `DataHandler` will hanlde downloading and uploading both general and recurring expenses to `data.txt` file. | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe prof recently announced on Canvas that non-standard icons should be removed from the DG. Might have just been too recent an announcement but remember to change it :)
docs/DeveloperGuide.md
Outdated
| ### Summary of Expenses | ||
|
|
||
| #### Sequence Diagram | ||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current sequence diagram looks quite complicated, perhaps you could just focus on the SummaryCommand and ExpenseReporter classes instead of showing the complete process from user input to final output since most of the detail is focused at ExpenseReporter.
docs/DeveloperGuide.md
Outdated
| ``` | ||
|
|
||
| Step 5. Alternatively, the user executes `/summary` command to view the overall summary of the current expenses. | ||
| The `/summary` command calls `ExpenseReporter#listAllCategoryTotals()`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a method call? Why is it "#" and not "." ?
docs/DeveloperGuide.md
Outdated
|
|
||
| Given below is an example usage scenario and how the summary command behaves at each step. | ||
|
|
||
| Step 1. The user launches the application and adds some expenses into the application. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be easier to read if the steps numbers (Step 1, Step 2, etc.) are bolded or you just use the numbered list format.
docs/DeveloperGuide.md
Outdated
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sequence diagram has zero activation bars, is that intentional? I think some activation bars may be omitted but not all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, why are the arrows numbered? Might not match the course conventions.
|
|
||
| #### Sequence Diagram | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to other comment below, the sequence diagram is quite complicated. You could perhaps focus just on CommandExecutor onwards (to the right) to make it easier to understand. The outer layerson the left don't seem as important for this particular function.
Refactor Help Command to format help descriptions for recurring commands
ppp draft- sze ying
fix addRecurring unit test
Update Sequence Diagrams
Updated AddCommand UML diagrams and added PPP
…uts and outputs. Refactored some of the codes to keep a function to around 30 lines
created header comments for some classes explaining the function, inp…
Updated JavaDocs and PPP, minor edits to DG
Fix incorrect date bug
Update UG and add a max limit on the average command
Fix index too large bug
Make the code more defensive
Update PPP and add JUnit tests for FileDataParser
fixed hyperlink issues
changes some outputs to screenshots
FinTrek is a desktop app designed for university students to manage their expenses. It is optimized for use via the Command Line Interface (CLI) so that frequent tasks can be easily performed by keying in commands.