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-T13-2] Expense Tracker #79

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

Conversation

FanZixian
Copy link

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.​


Given below is an example usage of the feature.
![](./diagrams/AddFeature.png)

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.

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.

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.

@@ -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

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?

* [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

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`.

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
newline

|--------|----------|---------------|------------------|
|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 ... |

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 |

and github will format this automatically into a nice table
table

exposed in the `Parser` class.

Given below is an example usage of the feature.
![](./diagrams/AddFeature.png)

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

## Design & implementation

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

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.


Displayed below is a part of the class diagram for `CommandTotal`.
![](diagrams/TotalFeature.png)
Give below is an example usage of the feature.
Copy link

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."

Copy link

@Rayleigh47 Rayleigh47 left a comment

Choose a reason for hiding this comment

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

Overall good job!

Step 4. `CommandAdd#execute` instantiates a new `Expense` object with the returned `parsedInput[]` and adds it to
`expenseList`.

### "Delete" feature

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.

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)

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)

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.

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.

Copy link

@exetr exetr left a 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)
Copy link

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)
Copy link

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)
Copy link

Choose a reason for hiding this comment

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

The return line from monthFilter to MonthlyOverview is missing a closing bracket
image

FanZixian and others added 24 commits April 3, 2023 12:13
…pt "" input by default to set the category to uncategorized
…e, which makes this method more User friendly.
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.

10 participants