[CS2113-W14-1] W14G1 (WIAGI) #21
Conversation
src/main/java/seedu/classes/Ui.java
Outdated
| int sum = 0; | ||
| for (int i = 0; i < arrList.size(); i++) { | ||
| assert arrList != null : "ArrayList is null"; | ||
| int oneIndexedI = i + 1; |
There was a problem hiding this comment.
Perhaps augment your for-loop such that counter starts from 1 and ends at arrList.size().
This is apparent in other parts of the code as well.
| StringBuilder sbSpending = new StringBuilder(); | ||
| assert tag != null && !tag.isEmpty() : "Tag is null or empty"; | ||
|
|
||
| int tagsCount; |
There was a problem hiding this comment.
There is an naming inconsistency here for this 3 variables.
src/main/java/seedu/classes/Ui.java
Outdated
| filteredList.add(entry); | ||
| } | ||
| } | ||
| print_list(filteredList); |
There was a problem hiding this comment.
Method names should follow the camelCase, not snake_case.
src/main/java/seedu/classes/Ui.java
Outdated
| print_list(filteredList); | ||
| } | ||
|
|
||
| public static <T extends Type> void printMonthly(ArrayList<T> arrList) { |
There was a problem hiding this comment.
Is there a reason why the parameter is named with a generic name?
This issue is apparent in other parts of the code as well.
src/main/java/seedu/classes/Ui.java
Outdated
| return currDate; | ||
| } | ||
|
|
||
| private static boolean inRange(LocalDate date, LocalDate start, LocalDate end) { |
There was a problem hiding this comment.
The method name should start with a verb instead e.g. isInRange.
| private <T extends ArrayList<? extends Type>> void editList(String[] arguments, T list) | ||
| throws WiagiInvalidIndexException { | ||
| try { | ||
| Type toEdit = list.get(getIndex(arguments)); |
There was a problem hiding this comment.
Variable names used here are not descriptive.
| return; | ||
| } | ||
| String firstIndex = fullCommands[1]; | ||
| switch (firstIndex) { |
There was a problem hiding this comment.
Switch statement is a little long; perhaps break down this piece of code into different functions?
| } | ||
| String firstIndex = fullCommands[1]; | ||
| switch (firstIndex) { | ||
| case "tags": |
There was a problem hiding this comment.
There are other files that has already set the strings as constants, you could use it over here as well.
| "\t[3] Biweekly" + System.lineSeparator() + | ||
| "\t[4] Monthly"); | ||
| String userInput = Ui.readCommand(); | ||
| if (userInput.equals("1")) { |
There was a problem hiding this comment.
Consider using switch statement instead (just for aesthetic purposes).
|
|
||
| public class MonthlyRecurrence extends Recurrence { | ||
| @Override | ||
| public void checkIncomeRecurrence(Income recurringIncome, IncomeList incomes) { |
There was a problem hiding this comment.
Methods here are relatively long. Perhaps break it down into smaller parts?
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
There was a problem hiding this comment.
can use an association arrow from Type class to RecurrenceFrequency class instead
There was a problem hiding this comment.
Partly hard to reed the methods and attributes.
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
There was a problem hiding this comment.
parseRecurrence() is a static method as seen later on in the DG, should that method be underlined in the class diagram?
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
docs/DeveloperGuide.md
Outdated
|
|
||
| ### Recurrence Component | ||
|
|
||
| #### Motivation behind the component:</br> |
There was a problem hiding this comment.
the </br> is showing in the dg which i dont think was intended
docs/DeveloperGuide.md
Outdated
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
|
|
||
| ## Overall Class Diagram | ||
|  |
There was a problem hiding this comment.
I think since you are already omitting other classes for conciseness, i dont think its necessary and easier to read to include simple getter functions in the classes that you show here.
There was a problem hiding this comment.
especially since this is to show the overall architecture this can be quite trivial
docs/DeveloperGuide.md
Outdated
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> |
There was a problem hiding this comment.
the </br> is showing here also which i dont think is intended
docs/DeveloperGuide.md
Outdated
| Within Wiagi constructor, Storage class is constructed, which will load and initialise incomes, spendings and | ||
| password by de-serialising the text at their distinct file paths. Wiagi will then initialise it incomes and spendings | ||
| based on the member in the Storage class. | ||
|  |
There was a problem hiding this comment.
There's a ref for login but there is no sd
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
There was a problem hiding this comment.
This diagram is really quite hard to read, especially with all the long and similar function names for daily monthly and yearly recurrence, maybe there are other ways you can display this nicer. or maybe split it up
There was a problem hiding this comment.
Great kickstart.
You could consider the following for the next round:
- Give more context to your sequence diagrams.
- Use more descriptive path conditions in your diagrams.
- Double check the usage of
as a lot of them are printed on the webpage of the PD - A couple of nitty gritty details I added as reviews.
docs/DeveloperGuide.md
Outdated
| {list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the | ||
| original source as well} | ||
|
|
||
| [Original Source](https://github.com/nus-cs2113-AY2425S1/tp) | ||
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} |
docs/DeveloperGuide.md
Outdated
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
|
|
||
| ## Overall Class Diagram | ||
|  |
There was a problem hiding this comment.
Great strategy to leave out the irrelevant classes for a high level overview
docs/DeveloperGuide.md
Outdated
| Within Wiagi constructor, Storage class is constructed, which will load and initialise incomes, spendings and | ||
| password by de-serialising the text at their distinct file paths. Wiagi will then initialise it incomes and spendings | ||
| based on the member in the Storage class. | ||
|  |
There was a problem hiding this comment.
Will the unnamed instances of IncomeListStorage etc. be ready for garbage collection after the load is finished? If not, maybe add a comment on how they will be reused later.
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
There was a problem hiding this comment.
Partly hard to reed the methods and attributes.
docs/DeveloperGuide.md
Outdated
| + `YearlyRecurrence`: Handles entries labelled as yearly recurring events | ||
|
|
||
| ##### parseRecurrence method | ||
| Class: `Parser` </br> |
There was a problem hiding this comment.
What do you mean here? Is it part of the Parser class? Why then is it located under the implementation of the Recurrence class?
Potentially you could help the reader understand the structure a bit more but not only relying on your class diagrams and sequence diagrams, but also by giving you test more structure.
There was a problem hiding this comment.
I guess what you want to say with the 'class' label is in which classes it is used.
Simply change the naming then to make this more clear.
docs/DeveloperGuide.md
Outdated
| ``` | ||
| Below illustrates the functionality of the checkIncomeRecurrence method through a sequence diagram </br> | ||
| </br> | ||
| <img src="./Diagrams/addRecurrenceEntry.png" alt="addRecurrenceEntry" width="700"/> </br> |
There was a problem hiding this comment.
Instead of using the actual operators, potentially use what it intuitively means instead. (e.g. next recurrence is due).
This could help to reduce the complexity of the diagram.
There was a problem hiding this comment.
I like that you used a note to explain CheckDate.
Maybe consider choosing a more descriptive variable name for CheckDate instead. Could help to improve the readability of the code and your diagrams because fewer additional comments or notes are relevant.
docs/DeveloperGuide.md
Outdated
| #### Implementation: | ||
| #### Recurrence class | ||
| The `Recurrence` class is a abstract class that provides the interface for checking `Income` and `Spending` and adding | ||
| recurring entries into the list. </br> |
docs/DeveloperGuide.md
Outdated
| Priorities: High (must have) - * * *, Medium (nice to have) - * *, Low (unlikely to have) - * | ||
|
|
||
| | Priority | As a ... | I want to ... | So that I can ... | | ||
| |----------|----------|------------------------------------------------------|--------------------------------------------------| | ||
| | *** | user | start and close the application | use it only when needed | | ||
| | *** | user | add my financial transactions | track the flow of my money | | ||
| | *** | user | categorise my entries as income and spendings | better understand my financials | | ||
| | *** | user | add income and expenditure categories | see my overall net gain or loss | | ||
| | *** | user | see all my spendings | know what I spent on | | ||
| | *** | user | delete my entries | correct my mistakes | | ||
| | *** | user | have a password to my account | protect my account information | | ||
| | ** | user | edit my incomes and spendings | correct my mistakes | | ||
| | ** | user | categorise my expenses | see what I spend on | | ||
| | ** | user | categorise my incomes | see where my savings come from | | ||
| | ** | user | read the amount of money left in my allocated budget | gauge how much to spend for the remaining period | | ||
| | ** | user | set expenses and incomes as recurring | do not need to manually add them each time | | ||
| | ** | Student | set budgets for each category of expense | make better financial decisions | | ||
| | * | user | be alerted when I overspend my budget | try to curb my spendings | | ||
|
|
There was a problem hiding this comment.
I like this part of the DG particularly! Well done
docs/DeveloperGuide.md
Outdated
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
|
|
||
| ## Overall Class Diagram | ||
|  |
There was a problem hiding this comment.
Is this the right way to add comments to your class diagram? You might want to put the note outside the box and then have a dotted line pointing to the note from the box
docs/DeveloperGuide.md
Outdated
|
|
||
| ### adding of new entry | ||
|  | ||
| To add new entries, user will have to input the related commands. |
There was a problem hiding this comment.
The text is slightly buggy here. You might want to create a new line below the image
docs/DeveloperGuide.md
Outdated
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> | ||
| </br> |
There was a problem hiding this comment.
Your line break does not show correctly in the DG. You might want to try something else
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:</br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="800"/> |
There was a problem hiding this comment.
If I understand correctly, Spending and Income inherits from Type. But what is Type? Perhaps there could be more clarity if the parent class had a proper name
docs/DeveloperGuide.md
Outdated
| Within Wiagi constructor, Storage class is constructed, which will load and initialise incomes, spendings and | ||
| password by de-serialising the text at their distinct file paths. Wiagi will then initialise it incomes and spendings | ||
| based on the member in the Storage class. | ||
|  |
There was a problem hiding this comment.
Consider to adjust the background colour since the lifelines appear to be invisible.
docs/DeveloperGuide.md
Outdated
| </br> | ||
| Illustrated below is the sequence diagram of the Recurrence Component: </br> | ||
| </br> | ||
| <img src="./Diagrams/recurrenceSequenceDiagram.png" alt="recurrenceSequenceDiagram" width="700"/> |
There was a problem hiding this comment.
Rectangles should be sharp border as curved border is non-standard. And use dotted lines instead of quotation
There was a problem hiding this comment.
Maybe Parser should have a dependency on Type since it uses it in the static method?
docs/Diagrams/storageSave.png
Outdated
There was a problem hiding this comment.
Maybe there was a typo and there's a extra space for SpendingList for the first method?
There was a problem hiding this comment.
Maybe could look to add () for printStatisticsRequired if it is a method?
There was a problem hiding this comment.
Looks good. Maybe can indicate the access modifiers for password member under Storage since everything else has too?
docs/DeveloperGuide.md
Outdated
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
|
|
||
| ### Overall Class Diagram |
There was a problem hiding this comment.
If the purpose is to show the architecture, perhaps use an architecture diagram instead of a class diagram with a tag showing that "many of the classes are omitted for conciseness".
Then, break down your current class diagram into smaller diagrams per component.
Should there be usage of association labels on dependencies?
Also do include the remaining roles for associations.
Do update all the multiplicities of all classes.
There are some member attributes (e.g. password in Storage class) that does not have its access modifier.
docs/DeveloperGuide.md
Outdated
| which will initialise the variables in `Storage` respectively. | ||
|
|
||
| #### save method in `IncomeListStorage` `SpendingListStorage` | ||
| <img src="./Diagrams/Storage/saveStorageSequenceDiagram.png" alt="saveStorageSequenceDiagram" width="600" height="400"/><br> |
There was a problem hiding this comment.
You may want to consider omitting method calls to JavaAPI such as FileWriter, only if you feel that it is not of importance.
docs/DeveloperGuide.md
Outdated
| `10.0|part time|2024-10-10|job|MONTHLY|2024-10-10|10` | ||
|
|
||
| #### load method in `IncomeListStorage` `SpendingListStorage` | ||
| <img src="./Diagrams/Storage/loadStorageSequenceDiagram.png" alt="loadStorageSequenceDiagram" width="600" height="400"/><br> |
There was a problem hiding this comment.
Would there be a self-invocation for load() methods within the Storages?
Perhaps use UML notation for sequence diagrams instead of having "labels" to state what it is doing.
This problem exists in multiple sequence diagrams.
docs/DeveloperGuide.md
Outdated
| `10.0|part time|2024-10-10|job|MONTHLY|2024-10-10|10` | ||
|
|
||
| #### load method in `IncomeListStorage` `SpendingListStorage` | ||
| <img src="./Diagrams/Storage/loadStorageSequenceDiagram.png" alt="loadStorageSequenceDiagram" width="600" height="400"/><br> |
There was a problem hiding this comment.
Why is there an activation bar on :Wiagi if there are no methods called for the instance.
This problem exists in multiple sequence diagrams too.
docs/DeveloperGuide.md
Outdated
|
|
||
| Illustrated below is the class diagram for the Recurrence Component:<br> | ||
| <br> | ||
| <img src="./Diagrams/Recurrence/recurrenceClassDiagram.png" alt="recurrenceClassDiagram" width="950"/> |
There was a problem hiding this comment.
Are there any classes that have a dependency on RecurrenceFrequency?
docs/DeveloperGuide.md
Outdated
| ``` | ||
| Below illustrates the functionality of the checkIncomeRecurrence method through a sequence diagram <br> | ||
| <br> | ||
| <img src="./Diagrams/Recurrence/addRecurrenceEntry.png" alt="addRecurrenceEntry" width="800"/> <br> |
There was a problem hiding this comment.
The activation bar for incomes:IncomeList and :Recurrence seems unreasonable as there are no method calls.
Are there any lifeline ending?
docs/DeveloperGuide.md
Outdated
|
|
||
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
|
|
||
| ### Overall Class Diagram | ||
|  |
There was a problem hiding this comment.
I agree that an architecture diagram might be more effective in displaying a clearer high-level overview of how the system components interact with each other. The detailed information in the class diagram, while useful, may be more than necessary for showing the overall structure and interactions of the system’s major components.
docs/DeveloperGuide.md
Outdated
| At the end of the run, or when the user exits the application, `Wiagi` will save the lists. | ||
| Now let's delve deeper into some of these classes used for the program below | ||
|
|
||
| #### EntryType Class |
There was a problem hiding this comment.
descriptions for each class are generally clear and comprehensive, well done :)
docs/DeveloperGuide.md
Outdated
| + For missing files (e.g., new users), users will be prompted to set a new password at the start of the | ||
| program. The entered password will then be hashed and stored in a newly created password file. | ||
|
|
||
| #### Implementation: |
There was a problem hiding this comment.
formatting - should 'implementation' have a higher heading hierarchy?
docs/DeveloperGuide.md
Outdated
| which will initialise the variables in `Storage` respectively. | ||
|
|
||
| #### save method in `IncomeListStorage` `SpendingListStorage` | ||
| <img src="./Diagrams/Storage/saveStorageSequenceDiagram.png" alt="saveStorageSequenceDiagram" width="600" height="400"/><br> |
There was a problem hiding this comment.
are all the return arrows necessary? should any be omitted to reduce clutter and improve clarity??
docs/DeveloperGuide.md
Outdated
| ``` | ||
| Below illustrates the functionality of the checkIncomeRecurrence method through a sequence diagram <br> | ||
| <br> | ||
| <img src="./Diagrams/Recurrence/addRecurrenceEntry.png" alt="addRecurrenceEntry" width="800"/> <br> |
There was a problem hiding this comment.
activation bars should begin with a method call. here, the activation bar under incomes:IncomeList has no method call. This problem occurs in some other sequence diagrams as well
Fix bug for double overflow and not enclosed attribute
…into branch-ug
UG add delete command to command summary, DG refactor commands and add command class diagram
Login, editting data file update UG
Refactor DG
Rename filewriter and update storage SD
Edit my PPP, added javadocs, adding more test later for storage
Rename list income and list spending command
final final DG
PPP update
edit jar name
Minor edits to manual testing
Add author tags
WIAGI aims to help students who are beginning their financial independence journey and want to gain control over their budgeting, saving, and investing habits. We provide students with a simple to use application that offers a wide range of essential tools and features such as budgeting, saving, and investment analysis, helping them to make better financial decisions and be in control of their finances.