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-T14-3] MyLedger #31

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

Conversation

dsicol
Copy link

@dsicol dsicol commented Mar 2, 2023

This product is for university students who prefer CLI over GUI to manage their finances while in University, as this is when student income is minimal.

kyrixn added a commit to kyrixn/tp that referenced this pull request Mar 14, 2023
vishnuvk47 pushed a commit to vishnuvk47/tp that referenced this pull request Mar 15, 2023
…teAccount

Branch created a  createAccount method Ps will fixed ioredirection as program grows

Choose a reason for hiding this comment

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

Alignment of the saveList() and saveExpenditureList to the start of the activation bar can be improved.

Choose a reason for hiding this comment

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

Control of the diagram becomes ambiguous after addExpenditure(). The activation bars are mostly too long and not properly aligned.

Choose a reason for hiding this comment

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

When the method is returned, the line should be dotted instead of solid. Alignment of the activation bars can be further improved.

Copy link

@thomasjlalba thomasjlalba left a comment

Choose a reason for hiding this comment

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

I think the Developer Guide is quite in depth and seems to explain quite well. Just some minor issues here and there in terms of the diagrams. I think if it is possible can reformat the pictures as there are some pictures that are very long and so its very small on the Github page. 🥇

The following shows the UML diagram used for the parser component implemented in MyLedger.

<p align="center">
<img src="team/images/parserOverview.png">

Choose a reason for hiding this comment

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

At the bottom of the picture, there would need to be a closure in the activation bar of :MainInputParser, it seems like currently there is no closure

The following shows the UML diagram used for the parser component implemented in MyLedger.

<p align="center">
<img src="team/images/parserOverview.png">

Choose a reason for hiding this comment

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

The picture in the Github Page also seems to be cut out (on the right side).

updated with all the current expenditures in the expenditure array list.

<p align="center">
<img src="team/images/saveList.png">

Choose a reason for hiding this comment

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

I don't think the method call does not need to be underlined

The sequence diagram below shows the interactions of a successful execution of the EditCommand

<p align="center">
<img src="team/images/parserEdit.png">

Choose a reason for hiding this comment

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

The termination (X) should be after the activation bar (by a dotted line), I don't think it should be terminated right at the end of activation bar.

Copy link

@yixuann02 yixuann02 left a comment

Choose a reason for hiding this comment

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

Overall, very in-depth explanation of diagrams, good job! But do take note of the multiple notation errors in your diagrams. Well done.

Comment on lines +148 to +153
<p align="center">
<img src="team/images/UMLClassDiagramExpenditure.png" width="80%">
<br/>
<i>Figure 3: UML diagram for the Expenditure Categories component</i>
</p>

Choose a reason for hiding this comment

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

Very clear and easy to read UML Diagram provided. Also, good explanation of the diagram.

Comment on lines 186 to 192
Below shows the UML diagram representing the `command` package.
<p align="center">
<img src="team/images/umlCommandClassDiagram.png" width="100%">
<br/>
<i>Figure 4: UML diagram for the command package</i>
</p>

Choose a reason for hiding this comment

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

Perhaps you could split the UML diagram into different parts. The UML diagram is too small and unreadable.

Comment on lines 200 to 204
<p align="center">
<img src="team/images/saveList.png">
<br/>
<i>Figure 5: Sequence diagram for TxtFileStatus</i>
</p>

Choose a reason for hiding this comment

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

The activation bar of a method cannot start before the method call arrives and a method cannot remain active after the method has returned. Do take note of this notation error.

Comment on lines 214 to 219
<p align="center">
<img src="team/images/initializeList.png">
<br/>
<i>Figure 6: Sequence diagram for the process of saveExpenditureList</i>
</p>

Choose a reason for hiding this comment

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

Similarly, the return for the ExpenditureList, Expenditure etc. should not be before the end of activation bar, and also do take note of the notation errors mentioned above.

Copy link

@jaredoong jaredoong left a comment

Choose a reason for hiding this comment

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

Overall clear developer guide with detailed and clear diagrams, just need to fix minor formatting error. Good job!

The following shows the UML diagram used for the parser component implemented in MyLedger.

<p align="center">
<img src="team/images/parserOverview.png">

Choose a reason for hiding this comment

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

Image is cut off on the right side. Additionally, the figure is a little small due to the multiple options shown. One possible workaround could be just to include only 2 options just to show an example, while clearly stating that the other options are omitted from the diagram to prevent cluttering it.


Below shows the UML diagram representing the `command` package.
<p align="center">
<img src="team/images/umlCommandClassDiagram.png" width="100%">

Choose a reason for hiding this comment

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

Good that .png format was used which ensured that the image remained clear even when zooming in to see the details. However, the details are quite small, so you might want to consider splitting it into different diagrams

Comment on lines 101 to 102
the mark, unmark and edit commands. This is because they have a similar sequence diagram as the functions parseAdd and
parseLendBorrow. The only difference is the condition, with the loop happening one, one and four time(s) respectively.

Choose a reason for hiding this comment

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

The formatting for parseAdd and parseLendBorrow could be changed to standardize with the top portion.

`Expenditure`:
- Fields: `date`, `amount`, `description`

`AcademicExpenditure`**`:

Choose a reason for hiding this comment

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

Additional **, might be a typo

The sequence diagram below shows the interactions of a successful execution of the EditCommand

<p align="center">
<img src="team/images/parserEdit.png">

Choose a reason for hiding this comment

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

Activation bar for getExpenditure(index) should start at the head of arrow instead of above it. The 'X' that represents termination could also possibly be shifted down to below the activation bar.

- Navigate to the folder in command terminal and run the command `java -jar [filename].jar`
- Alternatively, double click on the JAR file to run the app.
#### Adding a record
1. Adding an expenditure

Choose a reason for hiding this comment

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

Not sure why but the bullet point for this is not showing up correctly on the github pages, but you could possibly include a new line before this, which could fix the error.
image



- Test case : `viewtype swimming`
- Expected : Invalid message will be shown with the respective error message, in this case an

Choose a reason for hiding this comment

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

For each of the error message, it would be good to add an example to give the user a clearer idea on what the error message would look like

TzeLoong and others added 24 commits April 5, 2023 23:15
Bug fixes & Improve code quality & Add PPP
- Fix bug where next repeat date resets when reading from save file
- Add repeat date field to constructors of tuition and accommodation expenditures
- Add new repeat date field for save file
- Refactor code that processes the repeat date for tuition and accommodation expenditures
dsicol and others added 30 commits April 10, 2023 22:46
Remove numbering from PPP
<br> Should not have errors anymore
Fix style in PPP and create JAR
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.

8 participants