-
Notifications
You must be signed in to change notification settings - Fork 233
[AY2425S2-CS2113-F11-2] OSPS #40
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
base: master
Are you sure you want to change the base?
[AY2425S2-CS2113-F11-2] OSPS #40
Conversation
2f8c9e1 to
4f5925b
Compare
Add Junit tests for friends commands
aditichadha1310
left a comment
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!
| System.out.println("Group created successfully!"); | ||
| } | ||
|
|
||
| public void viewGroup() { |
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.
This is a very long method. Generally, keep the method length to about 30 LOC
| * [x] Cancel the operation. | ||
| * The computed owed amounts are appended to a file. | ||
| */ | ||
| public void executeSplit() { |
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.
Another very long method, try to keep it short (<=30 LOC)
| return; | ||
| } | ||
|
|
||
| if(intMethod == 1) { |
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.
Avoid the usage of Magic numbers to maximize readability.
| String line = scanner.nextLine().trim(); | ||
| String[] parts = line.split("\\" + SEPARATOR); | ||
|
|
||
| // When a new group is encountered, create a new group object |
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.
Write comments for the reader to maximize readability. Avoid comments that are for your own/coder's reference.
| for (Map.Entry<Categories, List<String>> entry : categoryKeywords.entrySet()) { | ||
| for (String keyword : entry.getValue()) { | ||
| if (title.contains(keyword) || description.contains(keyword)) { | ||
| categoryCount.put(entry.getKey(), categoryCount.get(entry.getKey()) + 1); | ||
| categorized = true; | ||
| break; | ||
| } |
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.
Avoid arrow-head style code as much as possible
Update DG
Update README.md to include PPP link
Update PPP Link
Update PPP Link
KashfyZul
left a comment
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.
In general, a well-elaborated DG sufficiently explaining each feature/functionality
| ### 4.1 Application Class Diagram | ||
|
|
||
| * *glossary item* - Definition | ||
| ### 4.2 Expense CRUD 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.
The UML diagram is clear enough and makes sense for the most part. The description above the UML diagram states that "The main application class calls the constructor for the UI class, which calls its own method processCommand()" . Should the diagram reflect the UI class being constructed and calling its own method in a separate activation bar?
| --- | ||
|
|
||
| ## 3. Design | ||
|
|
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.
More visuals could be used in the DG for the various methods. For certain methods, it would be helpful to show what the expected output of a method were to look like
| - Valid expense states | ||
|
|
||
| ### 3.1.5 FriendsCommands Class | ||
|
|
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.
No description shown for this class
| - Writes the newCurrency string to the file. | ||
| - Closes the FileWriter after writing the data to ensure resources are properly released. | ||
| - Declares the IOException to be thrown, which is handled by the calling method. | ||
|
|
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 this section in general, I feel that more examples (e.g. sample inputs/outputs) could be included to help depict the functionalities of the various features more clearly. More indentations could also be considered to help differentiate between sections/subchapters
rchlai
left a comment
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, you did a pretty amazing job outlining every method, class, and interaction in the DG.
| ## Quick Start | ||
|
|
||
| 1. Ensure you have Java 17 or above installed on your PC. [Version 17 is preferred] | ||
| 2. You may download [here](https://se-education.org/guides/tutorials/javaInstallationMac.html) for Mac users and [here](https://www.oracle.com/sg/java/technologies/downloads/) for Windows users. | ||
| 3. If you have it installed already, you may check it by running “java -version” in your terminal. | ||
| 4. Download the latest .jar file from here. [link will be updated once v1 is ready] | ||
| 5. Copy the file to the folder you want to use as the home folder for your O\$P$ budget tracking app 🙂. | ||
| 6. Open a command terminal, cd into the folder you put the jar file in, and use the “java -jar oSpS.jar” command to run the application. | ||
| 7. Type the command in the command box and press Enter to execute it. | ||
| 8. EG: Typing “help” and pressing Enter will open a mini window showing a list of all possible commands. | ||
| 9. Refer to the [features](https://docs.google.com/document/d/125Cg7wzuc4XFo3wsziwL2f64KN1uUfvFL5dIm6IQrSk/edit?tab=t.xl7ogrtj0a5q#heading=h.61o02m6y9xrc) section below for details on all commands and functionalities. |
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.
Great outline of instructions for quick start
| * [User Guide](UserGuide.md) | ||
| * [Developer Guide](DeveloperGuide.md) | ||
| * [About Us](AboutUs.md) |
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.
Should these links be deleted?
docs/DeveloperGuide.md
Outdated
| [1. Introduction](#1-introduction) <br> | ||
| [2. Setup Guide](#2-setup-guide) <br> | ||
| [2.1 Prerequisites](#21-prerequisites) <br> | ||
| [3. Design](#3-design) <br> | ||
| [3.1.0 UI Class](#310-ui-class) <br> | ||
| [3.1.1 DataStorage Class](#311-datastorage-class) <br> | ||
| [3.1.2 GroupStorage Class](#312-groupstorage-class) <br> | ||
| [3.1.3 Commands Class](#313-commands-class) <br> | ||
| [3.1.4 ExpenseCommands Classes](#314-expensecommands-classes) <br> | ||
| [3.1.5 FriendsCommands Class](#315-friendscommands-class) <br> | ||
| [3.1.6 SplitCommand Class](#316-splitcommand-class) <br> | ||
| [3.1.7 BudgetManager Class](#317-budgetmanager-class) <br> | ||
| [3.1.8 Expense Class](#318-expense-class) <br> | ||
| [3.1.9 Friend Class](#319-friend-class) <br> | ||
| [3.2.0 Group Class](#320-group-class) <br> | ||
| [3.2.1 GroupManager Class](#321-groupmanager-class) <br> | ||
| [3.2.2 Messages Class](#322-messages-class) <br> | ||
| [3.2.3 Summary Class](#323-summary-class) <br> | ||
| [3.2.4 ExpenseClassifier Class](#324-expenseclassifier-class) <br> | ||
| [3.2.5 Currency Class](#325-currency-class) <br> | ||
| [4. Overall Application Architecture](#4-overall-application-architecture) <br> | ||
| [4.1 Application Class Diagram](#41-application-class-diagram) <br> | ||
| [4.2 Expense CRUD Feature](#42-expense-crud-feature) <br> | ||
| [4.3 Create Group Feature](#43-create-group-feature) <br> | ||
| [4.4 Split Expense Feature](#44-split-expense-feature) <br> | ||
| [4.5 Change Currency Feature](#45-change-currency-feature) <br> | ||
| [4.6 Data Visualization Feature](#46-data-visualization-feature) <br> | ||
| [5. Appendix](#5-appendix) <br> |
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.
Nice effort providing direct links
| O\$P\$ is an expense-tracker application that allows | ||
| the user to keep track of their expenditure with ease! | ||
| Users can also manage expenses in a group setting with | ||
| simple splitting functions, as well as view analytics | ||
| of their spending across multiple categories. |
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 making the introduction concise.
| ### 3.1.0 UI Class | ||
|
|
||
| This class displays system output messages whenever an action has occurred or invoked by the user using the listed commands. | ||
|
|
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.
A simple class diagram can be used to show the methods of each component.
docs/DeveloperGuide.md
Outdated
| - Validates input fields (title, description, date, amount) | ||
| - Ensures date format is DD-MM-YYYY | ||
| - Prevents negative amounts | ||
| - Handles empty inputs gracefully | ||
| - Uses assertions to validate state |
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.
Explanations are brief and concise.
| - Throws `IndexOutOfBoundsException` if the index is invalid. | ||
|
|
||
| ### 3.1.8 Expense Class | ||
|
|
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.
Empty description
docs/DeveloperGuide.md
Outdated
| as shown in the diagram. | ||
|
|
||
| {Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing} | ||
|  No newline at end of file |
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 target users section is not found.
docs/DeveloperGuide.md
Outdated
| as shown in the diagram. | ||
|
|
||
| {Give instructions on how to do a manual product testing e.g., how to load sample data to be used for testing} | ||
|  No newline at end of file |
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 value proposition section is not found.
docs/DeveloperGuide.md
Outdated
| - Prompts the user to choose between entering their own exchange rate (option 1) or using an estimated exchange rate (option 2). | ||
| - Handles the user's input by checking if they input '1' or '2', guiding them to make a valid choice. | ||
| - If the user selects option 1, it asks for the currency code (based on ISO 4217 standard) and validates if the currency exists in the exchangeRates map. | ||
| - In option 1, the user is also prompted to enter a custom exchange rate, which is validated and parsed. | ||
| - If the user selects option 2, it retrieves the current exchange rate for the selected currency using the getExchangeRate() method. | ||
| - If a valid exchange rate is found for the new currency, it calculates the exchange rate relative to the current currency. | ||
| - Handles NumberFormatException gracefully and prompts the user to input a valid number if there are formatting issues. | ||
| - The method invokes editExpenseCurrency() to update the expense currency with the final exchange rate and the new currency. |
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.
This part could leave some blank lines between the bullet statements to enhance readability.
| ## Table of Contents | ||
|
|
||
| --- | ||
|
|
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.
not enough visuals, maybe screenshots of the command prompts would be easier for users to understand how the program works.
| ## Table of Contents | ||
|
|
||
| --- | ||
|
|
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.
Some methods are bolded while some arent, maybe try standardizing the format
| ## Table of Contents | ||
|
|
||
| --- | ||
|
|
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.
no description for FriendsCommands Class
| ## Table of Contents | ||
|
|
||
| --- | ||
|
|
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 setup guide could include troubleshooting tips and more detailed instructions for configuring the development environment , especially for less experienced users
Change PPP name to matthewyeo1.md
Update Expense CRUD Feature Sequence Diagram in DG
Add more Javadoc comments to authored code
| --- | ||
|
|
||
| ## 3. Design | ||
|
|
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 class diagram for each class will help in the visualisation of the class and how their components are linked
| ### 3.1.9 Friend Class | ||
|
|
||
| ### 3.2.0 Group Class | ||
|
|
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 are some empty descriptions
| ### 4.1 Application Class Diagram | ||
|
|
||
| * *glossary item* - Definition | ||
| ### 4.2 Expense CRUD 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.
The UML diagram is well detailed, maybe you can put it on top of the DG so the reader can understand how different classes are associated with each other
| - **Method:** `getName()` | ||
| - **Features:** | ||
| - Returns the group’s name. | ||
|
|
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 methods and classes are generally well detailed
docs/DeveloperGuide.md
Outdated
| [1. Introduction](#1-introduction) <br> | ||
| [2. Setup Guide](#2-setup-guide) <br> | ||
| [2.1 Prerequisites](#21-prerequisites) <br> | ||
| [3. Design](#3-design) <br> | ||
| [3.1.0 UI Class](#310-ui-class) <br> | ||
| [3.1.1 DataStorage Class](#311-datastorage-class) <br> | ||
| [3.1.2 GroupStorage Class](#312-groupstorage-class) <br> | ||
| [3.1.3 Commands Class](#313-commands-class) <br> | ||
| [3.1.4 ExpenseCommands Classes](#314-expensecommands-classes) <br> | ||
| [3.1.5 FriendsCommands Class](#315-friendscommands-class) <br> | ||
| [3.1.6 SplitCommand Class](#316-splitcommand-class) <br> | ||
| [3.1.7 BudgetManager Class](#317-budgetmanager-class) <br> | ||
| [3.1.8 Expense Class](#318-expense-class) <br> | ||
| [3.1.9 Friend Class](#319-friend-class) <br> | ||
| [3.2.0 Group Class](#320-group-class) <br> | ||
| [3.2.1 GroupManager Class](#321-groupmanager-class) <br> | ||
| [3.2.2 Messages Class](#322-messages-class) <br> | ||
| [3.2.3 Summary Class](#323-summary-class) <br> | ||
| [3.2.4 ExpenseClassifier Class](#324-expenseclassifier-class) <br> | ||
| [3.2.5 Currency Class](#325-currency-class) <br> | ||
| [4. Overall Application Architecture](#4-overall-application-architecture) <br> | ||
| [4.1 Application Class Diagram](#41-application-class-diagram) <br> | ||
| [4.2 Expense CRUD Feature](#42-expense-crud-feature) <br> | ||
| [4.3 Create Group Feature](#43-create-group-feature) <br> | ||
| [4.4 Split Expense Feature](#44-split-expense-feature) <br> | ||
| [4.5 Change Currency Feature](#45-change-currency-feature) <br> | ||
| [4.6 Data Visualization Feature](#46-data-visualization-feature) <br> | ||
| [5. Appendix](#5-appendix) <br> |
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.
Nice content table with links
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.
summary table with links looks neat!
| 4. Displays a divider after processing each command (if still running). | ||
| 5. If no input is available, it displays an empty input message and stops execution. | ||
|
|
||
| ### `processCommand()` |
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 be more consistent in header size
| ### 3.1.2 GroupStorage Class | ||
|
|
||
| User-created groups are stored here, each containing the names of the members to be assigned an expense. | ||
|
|
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.
Maybe can expand on what this does
docs/DeveloperGuide.md
Outdated
| It enables users to select an expense from a list and split that expense among a group either equally or through a manual assignment, either via absolute amounts or percentages. | ||
| --- | ||
|
|
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
| ### 3.2.0 Group Class | ||
|
|
||
|
|
||
| ### Group Class |
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.
Repeated Group Class header
| #### Removing Friends | ||
| - **Method:** `removeFriend(String friendName)` | ||
| - **Features:** | ||
| - Searches for the first friend with a matching name and removes them. | ||
| - Returns `true` if removal is successful; otherwise returns `false`. | ||
| - Note: Uses a for-each loop for removal, which may require caution regarding concurrent modifications. | ||
|
|
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 be more consistent with other methods documented 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.
Would the addition of diagrams help visualize how the classes interact with each other?
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.
Is this sequence diagram reflective of the extensive functionality that the team has implemented?
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 a developer guide, could the introduction be expanded on to be clearer about their value proposition?
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.
Written explanations for each class are well-written. It is very clear and makes it easy to understand the functionality of each class and expected behaviour
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.
Should the methods in each class be extracted from the "design" section and elaborated on in a separate "Implementations" section?
Amend export fn
… appropriately I wish you Gradle passes
Updated functionality of view-group and viewgroupdirect and the DG UG…
CHANGE EXPENSE STORAGEEEEEEEE
Update input file and help function
Update PPP and additional checksum checks for view-group and view-mem…
…into update-modify-txt-bug # Conflicts: # src/main/java/seedu/duke/commands/FriendsCommands.java # src/main/java/seedu/duke/messages/Messages.java # text-ui-test/ACTUAL.TXT # text-ui-test/EXPECTED.TXT
…into update-modify-txt-bug
…7/tp into update-modify-txt-bug # Conflicts: # src/main/java/seedu/duke/commands/FriendsCommands.java # src/main/java/seedu/duke/messages/Messages.java
Add checks to ensure program doesnt crash upon modifying groups.txt
Update ppp
ready for deployment

No description provided.