[CS2113T-T10-1] MediVault #29
Conversation
docs/DeveloperGuide.md
Outdated
| The rest of the program consist of four components. | ||
| * `Command`: Executes command based on the user input that is processed by `Utilities` | ||
| component. The list of commands can be found in our User Guide [here](UserGuide.md). | ||
| * `Utilities`: Contains important driver classes for MediVault | ||
| * includes `parser`, `ui`, `storage` and `comparators`. | ||
| * `Inventory`: Contains a collection of classes used by MediVault to represent | ||
| different medication information. | ||
| * `Errors`: Contains collection of classes that handles exceptions during execution of MediVault. |
There was a problem hiding this comment.
Perhaps a high level sequence diagram could be included for the other four components to demonstrate their interaction and the flow of executing a command.
|
|
||
| ### Command | ||
|
|
||
|  |
There was a problem hiding this comment.
Should the above public void execute() be public abstract void execute()? The Command class itself is abstract and it seems that every child class has the execute() method
There was a problem hiding this comment.
Like what @marcusbory mentioned, should the execute() method be an abstract method?
|
|
||
| Given below is the sequence diagram for the interactions within the main application logic. | ||
|
|
||
|  |
There was a problem hiding this comment.
Should this be create() instead of create?
Should there be a data type for userInput, userCommand, parameters, parameterValues, ie. userInput:String ?
Would it be better to include a ref block here? This would signify to the user that the actual execution sequence diagram will be provided in another segment.
|
|
||
| The sequence diagram for `AddOrderCommand` is shown below. | ||
|
|
||
|  |
There was a problem hiding this comment.
Would it be better to not include the self calls for this sequence diagram (ie. remove less important details)? This diagram is very long vertically.
An alternative suggestion would be to break this sequence diagram up and explain each section individually.
There was a problem hiding this comment.
This issue also applies to some of the other sequence diagrams, such as UpdateStockCommand, AddStockCommand.
There was a problem hiding this comment.
Like what @marcusbory mentioned, is it better to split the sequence diagrams, as the diagram is very long which makes it look complicated?
docs/DeveloperGuide.md
Outdated
|
|
||
| The sequence diagram for `AddPrescriptionCommand` is shown below. | ||
|
|
||
|  |
There was a problem hiding this comment.
Should there be ":" for static methods based on the above diagram?
docs/DeveloperGuide.md
Outdated
| #### AddPrescriptionCommand | ||
|
|
||
| MediVault creates an `AddPrescriptionCommand` object when CommandParser identifies `addprescription` or | ||
| `add` in `prescription` mode. |
There was a problem hiding this comment.
Is this line the continuation of the previous line?
| * This project comes with a GitHub Actions config files (in `.github/workflows folder`). When GitHub detects those | ||
| files, it will run the CI for your project automatically at each push to the `master` branch or to any PR. No set | ||
| up required. | ||
| ## Design |
There was a problem hiding this comment.
The section header of "Design" is not aligned with the rest of the sections. Is it due to no line separation between line 86 and 87?
docs/DeveloperGuide.md
Outdated
| `archive` keyword in `order` mode. | ||
|
|
||
| * MediVault archives order records by specifying a date. | ||
| * MediVault will remove only DELIVERED order records that have date <= specified date and output it into the file named |
There was a problem hiding this comment.
Is it necessary for "DELIVERED" to be in the upper case?
| * This project comes with a GitHub Actions config files (in `.github/workflows folder`). When GitHub detects those | ||
| files, it will run the CI for your project automatically at each push to the `master` branch or to any PR. No set | ||
| up required. | ||
| ## Design |
There was a problem hiding this comment.
As mentioned, the header is not aligned. There is extra whitespace before the header.
|
|
||
| The sequence diagram for `DeleteOrderCommand` is shown below. | ||
|
|
||
|  |
There was a problem hiding this comment.
Maybe consider using more reference diagrams to keep the diagrams concise and understandable.
docs/DeveloperGuide.md
Outdated
|
|
||
| The sequence diagram for `DeletePrescriptionCommand` is shown below. | ||
|
|
||
|  |
There was a problem hiding this comment.
Based on this diagram, its look like there are many nested ifs. Is the code having many nested if or is the diagram suppose to have different if statements that are not nested?
|
|
||
| The sequence diagram for `UpdatePrescriptionCommand` is shown below. | ||
|
|
||
|  |
There was a problem hiding this comment.
Don't think its necessary new when creating a new instance.
|
Overall, the developer guide is well-detailed with good diagrams but it is better to split it up to reduce complications. |
Claim ownership of purpose section in UG
…into branch-DispenseMedication # Conflicts: # docs/UserGuide.md
…ung/tp into branch-DispenseMedication # Conflicts: # docs/UserGuide.md
Fix bug addstock and addprescription. Fix bugs in PED. Update DG.
…into update-prescription
Fix PED bugs, modified DG and UG
Remove functionality from PPP
Shorten more PPP
Update PPP
Shorten PPP
Fix typo in DG
Update validator class diagram
Modify parameters checker SD
Include project overview into PPP
Include overview for PPP
Shorten PPP
Shorten PPP even more
Shorten PPP
Shorten PPP
Update readme
Update FAQ in UG for CS2101
MediVault is a Command Line Interface (CLI) application that will help to manage medication supplies within a pharmacy. It will solve the issue of tracking the stocks manually, discrepancies between actual and physical stocks as well as make the user's life easier by keeping track of expired medication.