-
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-T14-3] MyLedger #31
base: master
Are you sure you want to change the base?
Conversation
…teAccount Branch created a createAccount method Ps will fixed ioredirection as program grows
docs/team/images/saveList.png
Outdated
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.
Alignment of the saveList() and saveExpenditureList to the start of the activation bar can be improved.
docs/team/images/initializeList.png
Outdated
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.
Control of the diagram becomes ambiguous after addExpenditure(). The activation bars are mostly too long and not properly aligned.
docs/team/images/parserEdit.png
Outdated
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.
When the method is returned, the line should be dotted instead of solid. Alignment of the activation bars can be further improved.
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 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. 🥇
docs/DeveloperGuide.md
Outdated
The following shows the UML diagram used for the parser component implemented in MyLedger. | ||
|
||
<p align="center"> | ||
<img src="team/images/parserOverview.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.
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
docs/DeveloperGuide.md
Outdated
The following shows the UML diagram used for the parser component implemented in MyLedger. | ||
|
||
<p align="center"> | ||
<img src="team/images/parserOverview.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 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"> |
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 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"> |
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 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.
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, very in-depth explanation of diagrams, good job! But do take note of the multiple notation errors in your diagrams. Well done.
<p align="center"> | ||
<img src="team/images/UMLClassDiagramExpenditure.png" width="80%"> | ||
<br/> | ||
<i>Figure 3: UML diagram for the Expenditure Categories component</i> | ||
</p> | ||
|
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.
Very clear and easy to read UML Diagram provided. Also, good explanation of the diagram.
docs/DeveloperGuide.md
Outdated
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> | ||
|
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 you could split the UML diagram into different parts. The UML diagram is too small and unreadable.
docs/DeveloperGuide.md
Outdated
<p align="center"> | ||
<img src="team/images/saveList.png"> | ||
<br/> | ||
<i>Figure 5: Sequence diagram for TxtFileStatus</i> | ||
</p> |
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 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.
docs/DeveloperGuide.md
Outdated
<p align="center"> | ||
<img src="team/images/initializeList.png"> | ||
<br/> | ||
<i>Figure 6: Sequence diagram for the process of saveExpenditureList</i> | ||
</p> | ||
|
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, 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.
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 clear developer guide with detailed and clear diagrams, just need to fix minor formatting error. Good job!
docs/DeveloperGuide.md
Outdated
The following shows the UML diagram used for the parser component implemented in MyLedger. | ||
|
||
<p align="center"> | ||
<img src="team/images/parserOverview.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.
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.
docs/DeveloperGuide.md
Outdated
|
||
Below shows the UML diagram representing the `command` package. | ||
<p align="center"> | ||
<img src="team/images/umlCommandClassDiagram.png" width="100%"> |
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 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
docs/DeveloperGuide.md
Outdated
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. |
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 formatting for parseAdd
and parseLendBorrow
could be changed to standardize with the top portion.
docs/DeveloperGuide.md
Outdated
`Expenditure`: | ||
- Fields: `date`, `amount`, `description` | ||
|
||
`AcademicExpenditure`**`: |
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.
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"> |
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.
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.
docs/DeveloperGuide.md
Outdated
- 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 |
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.
docs/DeveloperGuide.md
Outdated
|
||
|
||
- Test case : `viewtype swimming` | ||
- Expected : Invalid message will be shown with the respective error message, in this case an |
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.
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
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
Enhance features
Reformat DG
Add page breaks for PPP
Finalized UG
Improve format of PPP
Remove numbering from PPP
new update formating
Final finalized DG
<br> Should not have errors anymore
more formatting DG
Fix style in PPP and create JAR
Reformat PPP
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.