[CS2113-T16-1] FinanceIt#48
Conversation
kaiwen98
commented
Oct 1, 2020
kaiwen98
commented
…g-Feature1withlogging Updated Logging and Assertion for weisiong Features 2:AddFunction
daniellimws
left a comment
daniellimws
left a comment
There was a problem hiding this comment.
Referring to Overall Architecture.png, is the relationship with ParamChecker supposed to be a dependency instead of association, since it is a Singleton?
|
Similar to my comment on Overall Architecture.png, in Handler_Commands.png, are the |
|
For the sequence diagrams, iirc the invocation needs to come from either an activation bar or from an actor. Like this taken from the textbook.
In the sequence diagrams in this DG, the first invocation comes from the lifeline, as I annotated in the image above. Not sure whether this is correct. |
There was a problem hiding this comment.
The developer guide looks quite comprehensive! It seems that every module in the codebase is well-documented with a lot of details, which helps me to know a lot about the system before even reading the code. 👍
I feel that it was sometimes hard to navigate because most of the headings are around the same level. Perhaps some whitespace or a horizontal line could be added to separate each section more clearly.
I also feel that for most sections, the level of the headings don't accurately represent the level of hierachy of the content, making the flow feel quite weird. For example under ### FinanceTools, every header is a level 3 header (###), i.e. ### Simple Interest Calculator and ### Parameters are also level 3 headers. But I got the impression that parameters should be one level below ### SimpleInterestCalculator (i.e. #### Parameters instead). So I am quite confused with regards to this.
I also face this problem with most of the other sections in the guide.
| ### Goal Tracker | ||
| ### Set Expense Goal Feature | ||
| The set expense goal feature is being implemented by ```GoalTracker```. It allows the user to set an expense goal for | ||
| the respective month to ensure that the user does not overspent his budget. | ||
| When user enter ```expense 2000 for 08```, the command will be sent to InputParser and parse it into String[]. | ||
| With the String[], it will be sent to a class called ```Goal```, and it will store the individual information. | ||
| Afterwards, it will be added to a ArrayList in a class called ```TotalGoalList```. |
There was a problem hiding this comment.
Should String[] and InputParser be in a code block (as in String[] and `InputParser)?
| ### How it works? | ||
| Firstly, user will input the command based on the `Format`. | ||
| Secondly, the input command will be sent to InputParser to parse. | ||
| Thirdly, the parsed information will be sent to class `Goal` to store the individual information | ||
| Next, it will be added to a ArrayList in class `TotalGoalList`. | ||
| Lastly, the goal status will be displayed to the user. |
There was a problem hiding this comment.
Should InputParser and ArrayList be in code blocks? (ArrayList and InputParser)
| * The initialisation of ```Ledger``` and ```Entry``` instances can be | ||
| performed with reference to input parameters supplied from the user input. | ||
| * For ledger creation operations, the input from the user is parsed and passed into an initialized ledger instance | ||
| to handle. That is, the handling of input parameters is abstracted out from the tracker classes. | ||
| <br> The handle operation will set the various attributes within the ledger in accordance to specifications inferred | ||
| from the user input. | ||
| <br> If the ledger is successfully specified in full, it will be added to a ```ledgerList``` instance within the handler | ||
| class ```ManualTracker```. | ||
| * For ledger deletion/open, a ledger will need to be selected from the ledger list maintained by the handler class. | ||
| <br>Hence, the input from the user is parsed and passed into a command instance to handle. If the input | ||
| is valid, the ledger list instance will assign a reference to the ledger selected to a public ```currLedger``` | ||
| attribute. | ||
| <br>After which, an operation of edit/open would be performed upon the ledger referenced from | ||
| ```currLedger``` in ```ledgerList```. |
There was a problem hiding this comment.
Maybe this is still work in progress. I feel that this would be easier to understand with relevant diagrams.
There was a problem hiding this comment.
Same here, I think something like a flow chart would be good in aiding understanding
| ``` | ||
| <command> <param type> <parameter> <param type> <parameter> ... | ||
| ``` | ||
| * The ```command``` string determines the current state of the Finite State Machine, and |
There was a problem hiding this comment.
For this reference to a Finite State Machine, I think it would be good to include some diagrams to help the reader (me) visualize it.
|  | ||
|
|
||
|
|
||
| |Module| Function | | ||
| |--------|----------| | ||
| | ```Parser```|Parse inputs from user and return ```CommandPacket``` instance with organised ```commandString``` and ```paramMap``` | ||
| | ```Tracker/ Handler```|Manages the overall workflow of the Manual Tracker; identifies operation required from input and executes the corresponding ```command```. | ||
| | ```Data``` |Refers to ```Ledger``` instances, stores relevant data of the day's transactions. | ||
| | ```Data List``` |Refers to ```LedgerList``` instances, maintains Ledger instances within the program. | ||
| | ```Commands``` |Processes information from ```CommandPacket``` and executes the appropriate process from recognised params. | ||
| | ```Logic``` |Outlines the abstract behavior of commands, as well as handle verification of params with appropriate error handling. |
There was a problem hiding this comment.
I feel that it would be good to have one sentence to give an overview of this section.
|  | ||
|
|
||
| |Class| Function | | ||
| |--------|----------| | ||
| |```retrieveLedgerCommand```| Process ```paramTypes```-```param``` pairs from the ```CommandPacket``` instance to identify specified ```Ledger``` instance, then retrieves the instance from the existing ```LedgerList```. | ||
| |```createLedgerCommand```| Process ```paramTypes```-```param``` pairs from the ```CommandPacket``` instance to identify specified ```Ledger``` instance to be created, then creates the instance and append to existing ```LedgerList```. | ||
| |```retrieveEntryCommand```| Omitted and left as exercise for reader. : ^ ) | ||
| |```createEntryCommand```| Omitted for brevity. | ||
| |```editEntryCommand```| Omitted for brevity. | ||
| |```ParamChecker```| Class contains a collection of methods that verify the correctness of the ```param``` supplied. <br><br> For instance, ```ParamChecker.checkAndReturnIndex``` checks if the index provided is out of bounds relative to the specified list, and throws the relevant exception if the input index is invalid. | ||
| |```ParamHandler```| Abstract class that outlines the general param handling behavior of ```commands``` instances and other classes that need to handle ```params``` in its operation. |
There was a problem hiding this comment.
I feel that it would be good to have one sentence to give an overview of this section.
|  | ||
|
|
||
| |Class| Function | | ||
| |--------|----------| | ||
| |```retrieveLedgerCommand```| Refer to section above. | ||
| |```createLedgerCommand```| Refer to section above. | ||
| |```retrieveEntryCommand```| Omitted for brevity. | ||
| |```createEntryCommand```| Omitted for brevity. | ||
| |```editEntryCommand```| Omitted for brevity. | ||
| |```ManualTracker```| Implements Manual Tracker. Contains handler methods that implements a particular operation capable by the Manual Tracker. <br><br> These methods use the above ```command``` instances for param handling operations from user input. | ||
| |```EntryTracker```| Omitted for brevity. |
There was a problem hiding this comment.
I feel that it would be good to have one sentence to give an overview of this section.
| |```DateTimeItem```| Abstract class that extends ```Item``` class; instances will have ```LocalDate``` or ```LocalTime``` attributes and corresponding helper methods. | ||
| |```Item```| Abstract class to define behavior of entities that need are stored in ```ItemList``` instances. | ||
|
|
||
| #### Functions with Sequence Diagrams |
There was a problem hiding this comment.
Perhaps something like "Details of implementation" would better represent this section as compared to "Functions with Sequence Diagrams"
| ### Parameters (Monthly Compound Interest Calculator) | ||
| * ```/a``` - Amount (Mandatory) | ||
| * ```/r``` - Interest Rate (Mandatory) | ||
| * ```/p``` - Number of Months (Mandatory) | ||
| * ```/d``` - Monthly Deposit (Optional) | ||
|
|
||
| The following class diagram shows how the Yearly/Monthly Compound Interest Calculator feature works: |
There was a problem hiding this comment.
I feel that a jump from parameters to class diagram is quite abrupt. Perhaps it would be good to add another h3 header.
| * Value: ```param``` | ||
|
|
||
| ##### InputParser | ||
| * A helper class. Parses the input string and returns a corresponding ```commandPacket```. |
There was a problem hiding this comment.
I'm actually quite confused as to whether commandPacket is suppose to be a property of a class or a class itself, seems like it is a class after reading at the ParamHandler component, but from this line it seems like a property of some other class.
| |```retrieveLedgerCommand```| Process ```paramTypes```-```param``` pairs from the ```CommandPacket``` instance to identify specified ```Ledger``` instance, then retrieves the instance from the existing ```LedgerList```. | ||
| |```createLedgerCommand```| Process ```paramTypes```-```param``` pairs from the ```CommandPacket``` instance to identify specified ```Ledger``` instance to be created, then creates the instance and append to existing ```LedgerList```. | ||
| |```retrieveEntryCommand```| Omitted and left as exercise for reader. : ^ ) |
There was a problem hiding this comment.
Classes should be written woth first letter of each internal word in upper case
|
|
||
| ##### Handler and Command | ||
|
|
||
|  |
There was a problem hiding this comment.
Is retriveEntryCommand, editEntryCOmmand, createEntryCommand properties of EntryTracker? Doesn't seem to be very clear based on the diagram as the table below doesn't really states how any of these EntryCommands are being used/ returned, etc. Also, is State a class or an Enumaration or? Would be better to make it clearer earlier
|
|
||
| ##### Handler and Parser | ||
|
|
||
|  |
There was a problem hiding this comment.
Might want to indicate the multiplicity clearer to make everything nicer
|
|
||
| ##### Handler and Data | ||
|
|
||
|  |
There was a problem hiding this comment.
I think a better option might be to put the aggregation symbol from LedgerList to Ledger and EntryList to Entry (by putting a shaded diamond at LedgerList in the arrow pointing from LedgerList to Ledger).
HengFuYuen
left a comment
HengFuYuen
left a comment
There was a problem hiding this comment.
A lot of effort has been put into the developer guide. Perhaps some changes might make it better but good job!
| ## Design & implementation | ||
|
|
||
| {Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.} | ||
| ### Summary |
There was a problem hiding this comment.
Perhaps consider adding in some architecture diagrams or class diagrams for the Summary and Architecture sections so it is easier for the reader to visualise the information.
| ##### Input Conventions | ||
| * The user input is composed of the following format: | ||
| ``` | ||
| <command> <param type> <parameter> <param type> <parameter> ... |
There was a problem hiding this comment.
Perhaps you can consider adding specific examples to aid reader understanding.
|
|
||
| ##### Command and Logic | ||
|
|
||
|  |
There was a problem hiding this comment.
Regarding this class diagram, based on the dashed line from the various commands to the ParamChecker, I am assuming that the command classes have dependencies with the ParamChecker. In this case, is showing multiplicities for dependencies correct? Perhaps you might want to consider removing it. You can also consider this change for the class diagram under the handler and parser section. Perhaps you may also want to consider capitalising the first letter of the command classes. Also, I am not too sure if the current notation of singleton is correct or not.
|
|
||
| ##### Handler and Data | ||
|
|
||
|  |
There was a problem hiding this comment.
Perhaps you can consider making the association between EntryList and Ledger more specific. Based on the methods and attributes, it seems that Ledger contains an EntryList only but the multiplicity both ways is 1, does this mean that EntryList also stores a Ledger?
|
|
||
| #### Functions with Sequence Diagrams | ||
|
|
||
| ##### Creation of Ledger |
There was a problem hiding this comment.
Perhaps you can consider having object diagrams for each step and you can also consider this for the other functions as well.
| <br /> | ||
| The following sequence diagram shows how the Simple Interest Calculator feature works: | ||
| <br /> | ||
| .png) |
There was a problem hiding this comment.
Perhaps you can consider retaining only the object box at the top. You can also consider this change for the diagram above this.
| ```FinanceTools.main()```. | ||
| <br /> | ||
|
|
||
| __Parameters (Yearly Compound Interest Calculator)__ |
There was a problem hiding this comment.
Perhaps you can consider combining the two similar parts in this section to reduce duplicates.
| The commands are stored before the params are handled and implementation is executed. The results from calculation | ||
| is stored when the implementation has finished executed. | ||
|
|
||
| #### Feature 3: Goal Tracker |
There was a problem hiding this comment.
Perhaps this is feature four maybe?
| * setExpenseGoal: expense 5000 for 08 | ||
| * setIncomeGoal: income 5000 for 08 | ||
|
|
||
| ##### How it works? |
There was a problem hiding this comment.
Perhaps consider using object diagrams.
| This sequence diagram will show the flow of setting of expense goal: | ||
|  | ||
|
|
||
| #### Feature 4: Save Manager |
There was a problem hiding this comment.
Perhaps consider including a higher level diagram.
Add warning for day of month between 29 and 31, since some month(s) do not have these day(s) Fix formatting and reword error message for clarity. Add excluded months into list for RecurringTracker
Bug fixes for RecurringTracker
Fixed UI bugs relating to table printing
Update manual testing section of dev guide.
Handle negative and alphabet inputs in FinanceTools
Update Table of Contents in UserGuide.md
Update DeveloperGuide.md
FIx bug in RecurringTrackerTest
Add page breaks
Add page breaks
Add page breaks for UserGuide.md
Add page breaks
Add page breaks for DeveloperGuide.md
Image Resize
Resized images
Saver Manager: Users can no longer enter special Characters
8 participants
