-
Notifications
You must be signed in to change notification settings - Fork 248
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-T13-2] Expense Tracker #79
base: master
Are you sure you want to change the base?
[CS2113-T13-2] Expense Tracker #79
Conversation
|
||
Given below is an example usage of the feature. | ||
![](./diagrams/AddFeature.png) |
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.
Consider describing a little bit about what's going on in the functions called, like use refs or something.
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
## Design | ||
![](diagrams/Overall.png) | ||
Our main `Duke` class is responsible for the instantiation and launch of our 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.
There is a formatting issue at this line. I think a double space is missing on line 14 to make it a new paragraph
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
## Design | ||
![](diagrams/Overall.png) | ||
Our main `Duke` class is responsible for the instantiation and launch of our 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.
Hmm, I see that your product is called Expense Tracker but the Main Class for this product is called Duke. Perhaps it would be better to rename the Class to fit the product? So that I as a developer won't be confused as to why the class is named Duke.
docs/DeveloperGuide.md
Outdated
@@ -4,30 +4,148 @@ | |||
|
|||
{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well} | |||
|
|||
* [JSON-java](https://github.com/stleary/JSON-java) | |||
* [three-ten-extra](https://www.threeten.org/threeten-extra/) | |||
* * Requesting and Parsing of data from API into Java |
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.
Appears to be a duplicated bullet point in this line, possible formatting error?
docs/DeveloperGuide.md
Outdated
* [JSON-java](https://github.com/stleary/JSON-java) | ||
* [three-ten-extra](https://www.threeten.org/threeten-extra/) | ||
* * Requesting and Parsing of data from API into Java | ||
* https://www.youtube.com/watch?v=lDEfoSwyYFg |
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 it would be beneficial if this link was a hyperlink, making it easier for readers to click on it.
- `CommandTotal#getTotal()` -- Returns the total in 2 decimal places. | ||
|
||
|
||
Displayed below is a part of the class diagram for `CommandTotal`. |
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.
Possible formatting error here, the image is in the middle of a the line instead of being on a new line and the "displayed below" is actually next to the image rather than above. Possible need to insert a newline using the multiple spaces at the end of the line method as the next line also starts immediately after the image rather than on a new line
docs/DeveloperGuide.md
Outdated
|--------|----------|---------------|------------------| | ||
|v1.0|new user|see usage instructions|refer to them when I forget how to use the application| | ||
|v2.0|user|find a to-do item by name|locate a to-do without having to go through the entire list| | ||
| Version | As a ... | I want to ... | So that I can ... | |
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.
Possible formatting error in the table, there is an extra blank column to the right of "So that I can..."
note that it is not necessary to match the whitespaces one-to-one for the table in markdown, as github will automatically format it
ie. you can do this:
| v1.0 | new user | add a new entry for my | - |
| v1.0 | user | delete specific expenses | - |
| v1.0 | user | add expenses with dates | track how much I spend each day |
| v1.0 | user| add expenses of different categories | keep track of what I spend on |
exposed in the `Parser` class. | ||
|
||
Given below is an example usage of the feature. | ||
![](./diagrams/AddFeature.png) |
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.
Perhaps it would be nice to standardize the fonts in the diagrams, as the words in some diagrams are bolded and the words in others are not bolded
docs/DeveloperGuide.md
Outdated
## Design & implementation | ||
|
||
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} |
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.
Perhaps this should be replaced with something along the lines of "This section describes the design and implementation of the product, with UML diagrams and code snippets where applicable." instead.
docs/DeveloperGuide.md
Outdated
|
||
Displayed below is a part of the class diagram for `CommandTotal`. | ||
![](diagrams/TotalFeature.png) | ||
Give below is an example usage of the feature. |
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.
Possible typo, should rephrase as "Given below is an example usage of this feature."
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 good job!
docs/DeveloperGuide.md
Outdated
Step 4. `CommandAdd#execute` instantiates a new `Expense` object with the returned `parsedInput[]` and adds it to | ||
`expenseList`. | ||
|
||
### "Delete" feature |
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.
Can consider standardizing the header for "Delete" feature whether to address it as ' ' or " " as the rest use singular quotation marks.
to extract `month` and `year` from user input. It then calls on `Monthly Overview` if both `month` and 'year' | ||
are not null, which makes use of `MonthFilter` to filter out expenses in that specific month and returns sum by | ||
category sorted in descending order before printing out the final overview in the intended format. | ||
|
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.
Formatting error with month
and 'year', I think can consider describing / explaining how YearFilter works besides just MonthFilter as both methods are called.
For the diagram I think there's a small typo for filteredExpenses(), otherwise good job, it looks quite detailed and readable.
|
||
|
||
Displayed below is a part of the class diagram for `CommandTotal`. | ||
![](diagrams/TotalFeature.png) |
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 can consider reformatting the text below the diagram and align it to make it neater.
|
||
Given below is an example usage scenario and how the 'delete' mechanism behaves at each step. | ||
|
||
![](./diagrams/DeleteFeature.png) |
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 diagram is missing the activation bar for Duke.
`expenseList.getExpenseList()` and `parser.extractIndex(input)`. | ||
|
||
Step 3. `CommandDelete#execute()` removes the expense at index specified by the user. | ||
|
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 a step by step approach to explain how the command works is a good idea and gives a clear summary of what the method is about, good job.
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 on the DG so far, continue to keep improving it. I have left some comments regarding certain issues. Primarily, do ensure that you provide sufficient elaboration both through text and diagrams such that they help to reduce ambiguity for audiences reading your DG.
|
||
Given below is an example usage scenario and how the 'delete' mechanism behaves at each step. | ||
|
||
![](./diagrams/DeleteFeature.png) |
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.
Consider extending this sequence diagram to also show what you mentioned in step 3 - how an expense is deleted, in order to add value to this diagram.
category sorted in descending order before printing out the final overview in the intended format. | ||
|
||
Given below is the sequence diagram to explain how the 'monthly overview' mechanism behaves. | ||
![](diagrams/MonthlyOverview.png) |
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.
While I am able to understand the role of yearFilter and monthFilter after reading, do consider adding further elaborations to ensure how both filters work.
category sorted in descending order before printing out the final overview in the intended format. | ||
|
||
Given below is the sequence diagram to explain how the 'monthly overview' mechanism behaves. | ||
![](diagrams/MonthlyOverview.png) |
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.
…into brachAccount
Branch ped
Branch help
…pt "" input by default to set the category to uncategorized
…e, which makes this method more User friendly.
Branch case sensitivity
Update UG and code comment
Update AboutUs
format DG
formatting
Add Comment
Update helper and Message file
fix noSuchElement upon terminating
check hasNextLine for scanner
Our app will help users easily open/close accounts, keep track of past transactions as well as transfer money from one account to another easily with built in currency converters. The system would also verify the validity of transaction requests before they are carried out.