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

[CS2113T-T12-3] Stonks XD #33

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

Conversation

KZQ1999
Copy link

@KZQ1999 KZQ1999 commented Sep 30, 2021

Stonks XD is an expense tracker that help computing students manage their finances and provide reminders and advice regarding their spending. It is optimized for CLI users so that frequent tasks can be done faster by typing in commands.

Copy link

@datn02 datn02 left a comment

Choose a reason for hiding this comment

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

Pretty nice project. But you could also focus more on the overall aesthetic of the DG.

Comment on lines 105 to 109
The image below illustrates the sequence diagram in the context of listing methods
which includes listExpense, listIncome and listFind.


![Untitled Diagram drawio (2)](https://user-images.githubusercontent.com/69465661/138629733-63b2a115-5405-4af5-8a74-4d18f51c8f96.png)
Copy link

Choose a reason for hiding this comment

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

Isn't the return arrows in the alt block be more like a square instead of a triangle?
image

Comment on lines 16 to 19
![](Architecture.drawio.png)

The __Architecture Diagram__ above explains the high-level design of the StonksXD app.
Given below is a quick overview of the main components of the application and how they interact with each other:
Copy link

Choose a reason for hiding this comment

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

This could look like a class diagram with the arrows and the box contains multiple sections. Maybe you should change it to another representation method to avoid confusion

Copy link

Choose a reason for hiding this comment

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

image

Comment on lines 119 to 121
The image below shows the sequence diagram of how the `AddExpenseCommand` class is used and the other classes involved with it as well.

![img_2.png](AddExpenseCommandSD.drawio.PNG)
Copy link

Choose a reason for hiding this comment

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

Is this part to be added? It doesn't show up on the github page
image

Comment on lines 273 to 275
- ####_Adding Income/ Expense entries_
1. Test Case: `add_ex d/DESCRIPTION a/AMOUNT c/CATEGORY`. </p>
Expected : Adds an expense item to the list. Displays confirmation message with timestamp.
Copy link

Choose a reason for hiding this comment

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

This part shows the hashtags (#) on the Github page. Is this intentional?
image

Comment on lines 32 to 45
`StonksXD` is the main class of the app. It has 2 main functions:
1. Upon opening the app, it loads saved data by calling `DataManager`. Before closing the app, it calls `DataManager` again to save data.
2. Runs a loop receiving new user input from `Ui` and passing it to `Parser`.

`StonksXD` &rarr; `DataManager`

`Ui` &rarr; `StonksXD` &rarr; `Parser`

<br>

`Parser` is the class responsible for interpreting the user input.
It ensures the appropriate input format, and passes the input data to the appropriate command.

`StonksXD` &rarr; `Parser` &rarr; `Command`
Copy link

Choose a reason for hiding this comment

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

Maybe you could provide a class diagram to show how those components interact with each other
Similar comments for other components description

Copy link

@arraysius arraysius left a comment

Choose a reason for hiding this comment

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

The developer guide has been well written and clear for the most part. Some sections require more explanation or updated diagrams to reduce reader confusion.

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

![](Architecture.drawio.png)

Choose a reason for hiding this comment

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

image

While looking at the architecture diagram, it is not entirely clear what the arrows and their directions mean between components. Maybe a note can be placed to explain the what the arrows and the direction mean.

Choose a reason for hiding this comment

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

image

The 3-way arrow between Command, Financial Tracker and Budget Manager gave me the first impression that all 3 can interaction with one another. But the explanation below says
ParserCommandFinancialTracker
ParserCommandBudgetManager
Maybe two separate arrows can be used to display that Financial Tracker and BudgetManager are does not interact with one another directly.

<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:
![](BudgetComponent.drawio.png)

Choose a reason for hiding this comment

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

image

The getLimit() method sounds like it should return a value back to BudgetManager. It might be a drawing bug where the return was left out. If not, maybe a better method name can be used.


The image below illustrates the sequence diagram in the context of saving data into `StonksXD_Entries.csv`.

![img_4.png](SavingFeatureSD.drawio.png)

Choose a reason for hiding this comment

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

image

I assume that write(data:String) is void method. I think that if there is no return value, the return line can be drawn but the return word is not required.

Choose a reason for hiding this comment

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

image

The lifeline should not exceed past the destruction symbol. Maybe the image can be edited to erase the extended lifeline or have a note to explain the limitation of the drawing and that it should not exceed.

which includes listExpense, listIncome and listFind.


![Untitled Diagram drawio (2)](https://user-images.githubusercontent.com/69465661/138629733-63b2a115-5405-4af5-8a74-4d18f51c8f96.png)

Choose a reason for hiding this comment

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

image
Should the return arrow here be square to adhere to the convention in class?


<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:

Choose a reason for hiding this comment

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

image
Apart from the return missing, perhaps the diagram is a bit complex and should be split into ref?

Choose a reason for hiding this comment

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

In the budget component diagram, no return signs for all methods. It is better to add them to show that the method is done and it is returned to the previous layer?


<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:

Choose a reason for hiding this comment

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

In the budget component diagram, no return signs for all methods. It is better to add them to show that the method is done and it is returned to the previous layer?


<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:

Choose a reason for hiding this comment

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

Perhaps the Xbudget class lifeline should not extend the opt frame? Since it is created only in opt and should be executed after it

Copy link

@ThaddeusLim99 ThaddeusLim99 left a comment

Choose a reason for hiding this comment

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

Overall a very good effort so far. Needs some standardisation and correcting for sequence diagrams. Use of sequence and class diagrams are quite relevant. Explainations are generally not too long and thus not very difficult to understand.


{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}
Source: https://www.baeldung.com/java-testing-system-out-println

Choose a reason for hiding this comment

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

Perhaps the links could be hyperlinks instead for users to access them more easily?

which includes listExpense, listIncome and listFind.


![Untitled Diagram drawio (2)](https://user-images.githubusercontent.com/69465661/138629733-63b2a115-5405-4af5-8a74-4d18f51c8f96.png)

Choose a reason for hiding this comment

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

The comment on isExpense() could be clearer. maybe resize it box to become bigger?


Below is a sequence diagram of the Budget component when `handleBudget` is executed:
![](BudgetComponent.drawio.png)

Copy link

@ThaddeusLim99 ThaddeusLim99 Oct 29, 2021

Choose a reason for hiding this comment

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

Is the final return arrow of the BudgetManager object correct? Should the arrow not be at the end of the activation bar instead of the middle of the activation bar?
returnError

<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:
![](BudgetComponent.drawio.png)
Copy link

@ThaddeusLim99 ThaddeusLim99 Oct 29, 2021

Choose a reason for hiding this comment

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

Perhaps the XBudget object box could be placed at the top along with the other object boxes for better standardization. Currently it looks a little messy.
boxes


<br>

Below is a sequence diagram of the Budget component when `handleBudget` is executed:
Copy link

@ThaddeusLim99 ThaddeusLim99 Oct 29, 2021

Choose a reason for hiding this comment

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

Perhaps it would be better to standardise whether the conditional boxes (such as alt and opt) will be behind or infront of the activation bar that it crosses over with.
activationBar


The image below illustrates the class diagram in the context of data saving and loading.

![img_3.png](DataManagerCD.drawio.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 better to add in the most used variables in the Class Diagram rather than leaving it empty? So that the user has a better idea on what kind of variables are in the class.

Copy link

@ThaddeusLim99 ThaddeusLim99 left a comment

Choose a reason for hiding this comment

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

Added some pictures for reference

AnShengLee and others added 30 commits November 8, 2021 21:43
…into branch-CurrencyTest

* 'master' of https://github.com/AY2122S1-CS2113T-T12-3/tp:
  Update
  Update user guide
  Update guides
  Update user guide
  User guide update
  Final commit
  Update 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.

10 participants