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-F08-2] HealthVault #12

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

Conversation

MingShun98
Copy link

HealthVault is a desktop app for managing doctor, nurse and patient information, optimised for use through the command line interface. This app is for the head nurse of a hospital, if the user can type fast, it is better than a traditional GUI app.

@MingShun98
Copy link
Author

HealthVault is a desktop app for managing doctor, nurse and patient information, optimised for use through the command line interface. This app is for the head nurse of a hospital, if the user can type fast, it is better than a traditional GUI app.


import seedu.duke.exceptions.DukeException;

public class duplicateIDException extends DukeException {

Choose a reason for hiding this comment

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

I detect that this code is problematic. According to the Bad practice (BAD_PRACTICE), Nm: Class names should start with an upper case letter (NM_CLASS_NAMING_CONVENTION).
Class names should be nouns, in mixed case with the first letter of each internal word capitalized. Try to keep your class names simple and descriptive. Use whole words-avoid acronyms and abbreviations (unless the abbreviation is much more widely used than the long form, such as URL or HTML).

@MingShun98 MingShun98 closed this Mar 21, 2021
@okkhoy
Copy link
Contributor

okkhoy commented Mar 30, 2021

Updated the name and reopened the PR

@okkhoy okkhoy reopened this Mar 30, 2021
@okkhoy okkhoy changed the title [CS2113-F08-2] HealthVault [CS2113T-F08-2] HealthVault Mar 30, 2021
Copy link

@JethroPhuah JethroPhuah left a comment

Choose a reason for hiding this comment

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

Left a review


###

### **UI component**

Choose a reason for hiding this comment

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

I think an accompanying visual diagram would be useful here?

2. The user input determines which type of functionalities will be
> accessed by the user.

**Staff Menu**

Choose a reason for hiding this comment

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

I feel like the explanation are too detailed and lengthy and the explanation is too unnecessarily long.


11. Data is written and saved.

**Viewing all Staff**

Choose a reason for hiding this comment

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

Too many examples ?

> amongst the classes. This will be elaborated in the Common Classes
> component

> (Insert UI UML diagrams)

Choose a reason for hiding this comment

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

Don't forget!

allows for a structure that helps store details from user input in the
format that we want to be saved.

[Objects:]{.ul}

Choose a reason for hiding this comment

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

Not sure what this is for - is it to control the css formatting? It seems to be rendering on the page directly, though
image

> Duke.run() calls UI.scanInput() to read the user's input from the
> command line.

2. The user input determines which type of functionalities will be

Choose a reason for hiding this comment

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

Good, comprehensive and detailed explanation for the general running of the application :)


add \[Staff ID\] \[name\] \[age\] \[specialisation\]

> Getting User Input

Choose a reason for hiding this comment

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

Very nicely detailed, but I think a UML diagram would be nice to have here, for a pictorial overview

UI, StaffUI classes will be accessed, and the following sequence of
actions is called to prompt execution result to user:

add \[Staff ID\] \[name\] \[age\] \[specialisation\]

Choose a reason for hiding this comment

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

Personally, it was difficult for me to identify what the input format was - in terms of the code required. Perhaps backtick formatting could be used to convey what the input commands are?

image

> PatientUI.deletePatientMessage() to print a message notifying
> the user that the Patient has been deleted.

5. Saving the current list of patients

Choose a reason for hiding this comment

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

These steps are very well detailed, but maybe consider trimming them to only include the important/essential parts? Considering that the method being explained is 'deleting a patient', maybe more focus could be drawn to the actual Patient and related classes rather than the Parser functions? Maybe there could be some reorganization so that Parser has a section explaining its implementation separately

image

a. Using the case statements in patientParse, the command find is
> identified.

b. patientParse then calls lengthCheck() and iDParser() to scan the

Choose a reason for hiding this comment

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

Same as above - maybe not important here!

Our application utilises many layers of abstraction which allows each
individual component to be self contained yet able to work with other
components. The Main Menu component allows direct access to other
components as shown in the diagram.

Choose a reason for hiding this comment

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

Diagram does not seem to be loading on the page.
image

distinct functionalities: Adding, Deleting and Listing. For Nurse
Schedules/ Patients / Staff related features, there is an additional
search functionality.

Choose a reason for hiding this comment

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

For the features description, I think sample inputs and outputs can be included for users to be understand how the function would work.

> handle any additional user commands that are inputted to access
> features within that particular instance.

### **Staff-related Features**

Choose a reason for hiding this comment

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

I think visual representations of the sequence of actions can be included to accompany the description.

@@ -1,3 +1,1140 @@
**Developer Guide**
===================

Choose a reason for hiding this comment

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

The DG is well written with many details about the functions and implementations. While the diagrams are lacking, you can still add them on later. I think the formatting of the DG can also be standardised to look neater but overall it was a well written documentation. Good job! And shoutout to MsKwek MingShun98!

Copy link

@RainyCodeWizard RainyCodeWizard left a comment

Choose a reason for hiding this comment

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

Overall have minor changes that needs to be changed

which will be elaborated on in the section [[Common
Classes]{.ul}](#common-classes)

###

Choose a reason for hiding this comment

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

image
There is an Extra "###" in the document. Please consider removing it.


Shared functionalities are placed within the Common Classes component
which will be elaborated on in the section [[Common
Classes]{.ul}](#common-classes)

Choose a reason for hiding this comment

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

image
It shows Common Classes {.ul} on screen. If the {.ul} is not necessary please remove them.

**Architecture**
----------------

![](media/image2.png){width="6.267716535433071in"

Choose a reason for hiding this comment

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

image
There is no image under the Architecture header

4. Returning to main menu

```{=html}
<!-- -->

Choose a reason for hiding this comment

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

image
Consider removing the html comment tag.

Comment on lines 1059 to 1070
### User stories

**Priority** **As a\...** **I want to\...** **So that I can\...**
-------------- -------------- ------------------------------------------------------- --------------------------------------------------
\* \* \* new user quickly refer to usage instructions quickly get on track with the workflow
\* \* \* user add a new staff/patient
\* \* \* user delete staff/patients remove entries i no longer need
\* \* \* user quickly look up schedules for both nurses and doctors plan my schedule better
\* \* \* user quickly look up inventory inventories plan what and when to restock our inventory supplies
\* user have the program recognize slight errors in typing have leeway working in a high-stress environment

###
Copy link

@iamakilahamed iamakilahamed Apr 1, 2021

Choose a reason for hiding this comment

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

image
It is to difficult to read this information. This could be due to formatting issues. Perhaps, adding "|" after each entry should help you solve this problem.
https://guides.github.com/features/mastering-markdown/

Comment on lines 986 to 995
Getting User Input

> User input add command which is processed by DrugInstance.run().

Creating Drug object with User Input

> DrugAction.addDrug() creates a new NurseSchedule object and is stored
> into an existing ArrayList\<Drug\> inventory which contains all the inventory
> objects.

Choose a reason for hiding this comment

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

image
The information under this section could have been presented in a better format. Perhaps of using ">" to add details of the features, I think you can use bulleted points or numbers to break these information into different steps.

**Architecture**
----------------

![](media/image2.png){width="6.267716535433071in"

Choose a reason for hiding this comment

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

image
There is no image here, possibly a rendering error?

\* \* \* user quickly look up inventory inventories plan what and when to restock our inventory supplies
\* user have the program recognize slight errors in typing have leeway working in a high-stress environment

###

Choose a reason for hiding this comment

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

There seems to be an additional "###". It also appeared in a few other areas in the document, remember to remove it!


### **Doctor Appointment-related Features**

[Implementation:]{.ul}

Choose a reason for hiding this comment

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

There is a few instances of such notations, "[description:]{.ul}". Does the notation have any significance? It would be good to put mention what the notation means in the glossary!

> called in the classes stated below.

[Actions:]{.ul}

Choose a reason for hiding this comment

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

Is this an example of an action or all method uses this action? Got a little confused here, would be good to elaborate what is this part for?

[Actions:]{.ul}

1. In the menu of Doctor's Appointment, command of "add D12345 Alex M
> 21012021".

Choose a reason for hiding this comment

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

Is the design of using ">" intentional? I thought it was hard to read when the command is broken up. Do you think "> add D12345 Alex M 21012021" is better?

@@ -1,3 +1,1140 @@
**Developer Guide**

Choose a reason for hiding this comment

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

The DG is well written with breakdown on architecture and implementation. I think the group can focus on improving the format of the information written. There seems to be an issue on the images as they were not rendered properly so I could not give comment on the use of visuals. Keep up the good work! Good Job :))

app is for the head nurse of a hospital, if the user can type fast, it
is better than a traditional GUI app.

### **Purpose and scope**

Choose a reason for hiding this comment

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

Good section explaining the purpose and context of this DG.

> what actions to perform. The actions to be performed are methods
> called in the classes stated below.

[Actions:]{.ul}

Choose a reason for hiding this comment

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

Having additional pictures will be good, as it is clearer for reader to understand

6. When the user types in a command,

a. Duke.run() calls UI.scanInput() to read the user's input from
> the command line.

Choose a reason for hiding this comment

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

Usage of the "a." here seems confusing since the rest of the DG does not follow a similar format.


3. deleteSchedule() loops through the arraylist of schedules and calls remove() to delete the specified schedule.

### **Doctor Appointment-related Features**

Choose a reason for hiding this comment

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

Consider utilising bullet points for this section. The formatting is rather confusing, when viewed on the site.

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.