Skip to content
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

[CS2113T-T09-1] MediTracker #24

Open
wants to merge 1,111 commits into
base: master
Choose a base branch
from

Conversation

kyuichyi
Copy link

@kyuichyi kyuichyi commented Mar 7, 2024

MediTracker is a Command Line Interface (CLI) app to track your medications. It gives you the capability to manage your medications taken on a daily basis such as indicating if you have taken your medication and when it is going to expire.

@kyuichyi kyuichyi changed the title [CS2113-T09-1] MediTracker [CS2113T-T09-1] MediTracker Mar 7, 2024
JeffinsonDarmawan added a commit to JeffinsonDarmawan/tp that referenced this pull request Mar 17, 2024
Added flower command into the help text and corrected a minor spellin…
yeozongyao pushed a commit to yeozongyao/tp that referenced this pull request Mar 20, 2024
Copy link

@Luo-Z-Y Luo-Z-Y left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general well done, however UML not done.


---

### List Medication Command
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
Perhaps you could consider only highlighting the code part instead of the whole sentence.

- execute(MedicationManager medicationManager) - Executes the list command and performs its specific task, -t.
```Where the task can be either 'list -t all' to list all medications or 'list -t today' to list medications for the day, which is divided into three categories -> Morning, Afternoon and Evening. ```

The 'list -t' command requires the following:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
You could potentially highlight the code part in this segment to make it more readable.

Copy link

@liuy1103 liuy1103 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall LGTM, but looks a little simplistic, perhaps more UML diagrams and illustrations for feature enhancements 😄

The 'search' command requires the following:
1. To be added.

### Utilising the argument parser
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a class diagram to illustrate this to provide visual feedback 😄



## Design & implementation
{Describe the design and implementation of the product. Use UML diagrams and short code snippets where applicable.}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe you could remove these documentation instructions as they are not necessary

### Export to JSON File

![img.png](images/JsonFileExport.png)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could draw a dotted arrow away from FileReaderWriter to represent the output from "List dailyMedicationList = FileReaderWriter.loadDailyMedicationData();" in your MediTracker class at line 101.

---

### Export to JSON File

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding more design sequence diagrams to describe the functions described in the design and implementation



### Simulated Time
Talk about the advaned feature. Offset based on the system time so that the user or developer does not have to worry about calculating the time and can just type in the time.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider fixing the typo "advaned" to "advanced" here.

Copy link

@JianJiaT JianJiaT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start but could use more UML diagrams and fix some issues


### Value proposition

{Describe the value proposition: what problem does it solve?}
MediTracker ensures that you would now forget your overall schedule on what time and day to take your medication.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be "ensures that you would not forget" instead of "ensures that you would now forget"?

Comment on lines 20 to 24
The add medication command extends from the Command parent class and implements the following operations:
- execute(MedicationManager, DailyMedicationManager, Ui) - Adds the medication object into the respective medication managers.
- setMedicineAttributes() - Sets the medicine object's attributes to be added to the medicine managers.
- assertionTest(MedicationManager, DailyMedicationManager) - Asserts that medicine has been added to both medication managers.
- parseStringToValues(Arguments) - Parses string input for medicine quantity and dosage into double type.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add sequence diagram or class diagram here to show the interactions and relationships since this seems to involve quite a lot of classes?

Comment on lines 40 to 45
3. 'today -m' to run printTodayMedications(List<Medication> medications, List<DailyMedication> subList, String period)
from the DailyMedicationManager
4. 'today -a' to run printTodayMedications(List<Medication> medications, List<DailyMedication> subList, String period)
from the DailyMedicationManager
5. 'today -e' to run printTodayMedications(List<Medication> medications, List<DailyMedication> subList, String period)
from the DailyMedicationManager
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps clarify what is the difference between inputting "today -m", "today -a" and "today -e"?

Comment on lines 129 to 131
|v1.0| user | search medications from existing medicine library | search medications locally to have a quick preview of them and their purpose without the web |
|v1.0| multipurpose user | Store data locally | Use the application and see the data even when offline between sessions |
|v1.0| user | know the list of medications I have added | have a quick overview of the medication list and check the quantity and expiry date of each medication |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps clarify what is a "multipurpose user" and how it is different from a normal user?

- execute(MedicationManager medicationManager) - Executes the list command and performs its specific task, -t.
```Where the task can be either 'list -t all' to list all medications or 'list -t today' to list medications for the day, which is divided into three categories -> Morning, Afternoon and Evening. ```

The 'list -t' command requires the following:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps specify that the following parameters need to follow the "list -t" command instead of preceding it or some other order?

Comment on lines 4 to 6

{list here sources of all reused/adapted ideas, code, documentation, and third-party libraries -- include links to the original source as well}

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps add a table of contents at the top of the DG?

|v1.0| user | search medications from existing medicine library | search medications locally to have a quick preview of them and their purpose without the web |
|v1.0| multipurpose user | Store data locally | Use the application and see the data even when offline between sessions |
|v1.0| user | know the list of medications I have added | have a quick overview of the medication list and check the quantity and expiry date of each medication |
|v2.0| user | find a to-do item by name | locate a to-do without having to go through the entire list |
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps remove this user story since it seems to be a remnant from the project template that does not seem to be relevant to this application?

tests have been added to ensure that certain attributes of the `Argument` classes do not clash with
one another. `HelpArgument` is automatically added when an `ArgumentList` object is instantiated.
Calling `ArgumentList.parse` with empty `String` or invoking with the argument `-h` will trigger the
help message to be printed on the console.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider explaining how "trigger" works.

the key. In order to query the returned data structure, you can utilise the same enum.

In order to utilise the argument parser,
1. Determine if the `Argument` variant already exist. If not, create a new class and extend the `Argument` class.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good explaination

e0958902 and others added 30 commits April 15, 2024 23:19
# Conflicts:
#	docs/DeveloperGuide.md
Update developer guide for page breaks and user stories ordering
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.