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-3] HalpMi #3

Open
wants to merge 648 commits into
base: master
Choose a base branch
from

Conversation

tanweili
Copy link

@tanweili tanweili commented Mar 3, 2022

The hospital management system is a CLI application that helps to manage all hospital data with ease and efficiency where the user is the hospital administrator.

Copy link

@GlendonNotGlen GlendonNotGlen left a comment

Choose a reason for hiding this comment

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

Great implementation of DG overall. The DG was easy to read and diagrams are very neat without being overly cluttered.

Comment on lines 280 to 295
## Glossary
* *FUllNAME* - Standard form for fullname of patients and doctors is a String value with no spaces
* *NRIC* - Standard form for nric of patients and doctors is a String value with no spaces
* *AGE* - Standard form for age is an int value more than 0
* *GENDER* - Standard form for gender of patients and doctors is a char value of "M" or "F"
* *ADDRESS* - Standard form for address is a String value with no spaces
* *DOB* - Standard form for date-of-birth is a String value with no spaces
* *SPECIALIZATION* - Standard form for specialization of doctors is String value with no spaces
* *MEDICINEID* - Standard form for medicine id is a String value with no spaces
* *MEDICINENAME* - Standard form for medicine name is a String value with no spaces
* *DOSAGE* - Standard form for dosage of medicine is an int value, standard unit milligrams
* *EXPIRY* - Standard form for expiry of medicine is a String value with no spaces
* *SIDEEFFECTS* - Standard form for side effects of medicine is a String value with no spaces
* *QUANTITY* - Standard form for quantity of medicine is an int value
* *APPOINTMENTID* - Standard form for appointment id is a String value with no spaces
* *APPOINTMENTDETAILS* - Standard form for appointment details is a String value

Choose a reason for hiding this comment

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

Great use of a glossary here! Really helps in understanding the DG better


The rest of the App consists of these components.

* [**`Manager`**](#manager-component): The Brain.

Choose a reason for hiding this comment

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

Not sure if calling The Brain would be appropriate? Can perhaps change it to main driver function or say 'where most of the logic is computed'?


### Architecture

![Architecture Diagram](diagrams/OverallArch.png)

Choose a reason for hiding this comment

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

Really great diagram that gives an overall architecture overview of your program


## User Stories
Priorities: High (must have) - `* * *`, Medium (nice to have) - `* *`, Low (unlikely to have) - `*`

Choose a reason for hiding this comment

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

I think the formatting of this legend can be neater? Overall great idea nonetheless!

Comment on lines 261 to 279
### Non-Functional Requirements
Device Environment:
* Must have Java 11 or higher installed in OS
* 32-bit or 64-bit environment
* Command Line Interface supported

Performance of app:
* Function offline, without the need for internet access
* Quick to launch and use
* No noticeable lag or delay in performance when running
* Intuitive and seamless for new users.
* Ability to export the data into a txt file to load on another OS

Reliability of app:
* Data files should be updated constantly and accurately, with no data loss
* Data records should be retrievable and readable
* Text inputs should produce similar results if utilised multiple times.
* Program should run without any forced-close error due to bugs
--------------------------------------------------------------------------------------------------------------------

Choose a reason for hiding this comment

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

Do becareful of formatting error. The sub headers here seem to be misaligned

@tanweili tanweili changed the title [CS2113T-T09-3] Hospital Management System [CS2113T-T09-3] HalpMi Mar 31, 2022
Copy link

@Ch40gRv1-Mu Ch40gRv1-Mu left a comment

Choose a reason for hiding this comment

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

Some diagrams are too complex and hard to read for reader. May suggest to hide some details. For example, there's no need to show all the methods of classes in the Class diagram? (Can use notation to inform you hide some unimportant methods instead)

Comment on lines +45 to +46
![Architecture Diagram](diagrams/OverallArch.png)

Choose a reason for hiding this comment

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

Why is there a component named Duke in the overall architecture while your product name is not Duke. Do you mean the main function?

Comment on lines +53 to +54
**`Duke`** has single method called `main` which is called upon launch. This initialises a new instance of a `Manager` class, and calls the `runLoop()`
method belonging to the Manager object.

Choose a reason for hiding this comment

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

Suggests to change the name of "Duke" to "Main" instead to avoid confuseness.


The rest of the App consists of these components.

* [**`Manager`**](#manager-component): The Brain.

Choose a reason for hiding this comment

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

Confusing expression. Explain what does this component do specific instead.

Comment on lines +63 to +68
* [**`Command`**](#command): Contains the changes or updates to be made.
* [**`UI`**](#ui-component): The UI of the App.
* [**`Parser`**](#parser): Breaks down user input into parameters accepted by the app and creates a Command Object.
* [**`Validator`**](): Checks if the input provided by the User is Valid.
* [**`Storage`**](#storage): Reads data from data files and writes data to data files, also stores in app memory.

Choose a reason for hiding this comment

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

Is this correct indentation? or Command, UI, Parser, Validator, Storage belongs to Helper? which does not make sense

Comment on lines 214 to 219
Currently, `AppointmentList` has the following methods:
* `AppointmentList#add` -- Appends a new `Appointment` to the list.
* `AppointmentList#remove` -- Removes an entry from the list by appointment id.
* `AppointmentList#edit` -- Edits a existing entry from the list by.
* `AppointmentList#view` -- Text display of all or selected appointments in the list.
* `AppointmentList#find` -- Find selected appointments using the criteria given.

Choose a reason for hiding this comment

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

May hide some unimportant detail but only state the noteworthy part instead.

Comment on lines +1 to +8
@startuml
'https://plantuml.com/sequence-diagram

alt add patient
Manager -> Command: addPatient
Command -> PatientList: add
else delete patient
Manager -> Command: deletePatient

Choose a reason for hiding this comment

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

Suggest to add a notation to declare that this is a minimum notation sequence diagram.

Comment on lines 8 to 9
Duke -> Manager : Manager()
activate Manager

Choose a reason for hiding this comment

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

Is this wrong notation for execution of constructor? The activation bar should jus under the class. You may miss "create Manager"

Comment on lines 11 to 13
Manager -> UI: UI()
activate UI
UI --> Manager

Choose a reason for hiding this comment

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

Same as the previous comments

Comment on lines +103 to +110
destroy Storage
destroy UI
destroy Parser
destroy Validator
destroy Manager
deactivate Duke
destroy Duke

Choose a reason for hiding this comment

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

So review all the execution of construction pls.

Comment on lines +9 to +12
List <|-- PatientList
List <|-- DoctorList
List <|-- MedicineList
List <|-- AppointmentList
Copy link

@Ch40gRv1-Mu Ch40gRv1-Mu Mar 31, 2022

Choose a reason for hiding this comment

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

The arrows are too messy. Moreover, I didn't see the value to express the association between Storage and all specific XYZList, which doesn't add value. Can just show the a specific case as example

deactivate Duke
destroy Duke

@enduml

Choose a reason for hiding this comment

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

Understand that there are many method calls in this sequence diagram, however the sequence diagram looks like there is alot going on. Perhaps it would be better to break it down into separate sequence diagrams and reference each other.

-validateQuantity(String): boolean
-validateSpecialization(String): void
}
@enduml

Choose a reason for hiding this comment

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

This class seem to contain alot of methods and attributes, and it looks lengthy. Perhaps it be better to include only important attributes and methods.

Manager -> Command: viewMedicine
Command -> MedicineList: viewMedicine
end
@enduml

Choose a reason for hiding this comment

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

Would any of the commands return any output or list? If it does, will be good to show a return arrow back to the main class that calls the commands.

+parseViewMedicine(String): Command
+parseViewPatient(String): Command
}
@enduml

Choose a reason for hiding this comment

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

Again, there seems to be too many methods displayed in this function, perhaps it will be better to reduce and display only important methods and attributes.

Copy link

@ngys117 ngys117 left a comment

Choose a reason for hiding this comment

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

Mostly seems fine. Just need to take note of static members/methods in class diagrams. Another comment is maybe consider following the lecture uml diagram notation for showing visibility and abstract classes.


The rest of the App consists of these components.

* [**`Manager`**](#manager-component): The Brain.
Copy link

Choose a reason for hiding this comment

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

I'm not sure if this description is helpful. Maybe be a bit more explicit with what the Manager actually does? From the diagram it seems like it acts as an intermediary between the User/Duke and the rest of the components.


#### `Validator`

![ValidatorClassUML](diagrams/ValidatorClassUML.png)
Copy link

Choose a reason for hiding this comment

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

Perhaps the color is too dark, making the black text hard to read.


#### `Storage`

![StorageClassUML](diagrams/StorageClassUML.png)
Copy link

@ngys117 ngys117 Mar 31, 2022

Choose a reason for hiding this comment

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

Maybe there are too many inheritance arrowheads and association arrows overlapping one another?

image

Consider changing the formatting so that the either the Storage or List class is at the bottom to remove the overlaps.


{Describe the target user profile}
###Target user profile:
Copy link

@ngys117 ngys117 Mar 31, 2022

Choose a reason for hiding this comment

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

Seems like the header formatting is showing in the actual webpage?
image

* prefers typing to mouse interactions
* is reasonably comfortable using CLI apps

###Value proposition:
Copy link

Choose a reason for hiding this comment

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

Same issue with header formatting here.

Comment on lines 267 to 279
Performance of app:
* Function offline, without the need for internet access
* Quick to launch and use
* No noticeable lag or delay in performance when running
* Intuitive and seamless for new users.
* Ability to export the data into a txt file to load on another OS

Reliability of app:
* Data files should be updated constantly and accurately, with no data loss
* Data records should be retrievable and readable
* Text inputs should produce similar results if utilised multiple times.
* Program should run without any forced-close error due to bugs
--------------------------------------------------------------------------------------------------------------------
Copy link

@ngys117 ngys117 Mar 31, 2022

Choose a reason for hiding this comment

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

Are the indentations for "Performance of app" and "Reliability of app" here intentional? Looks inconsistent with the "Device Environment" portion.


The Sequence Diagram below showcases the general Logic and Flow of the program from Launch till Exit.

![Sequence Diagram](diagrams/SequenceDiagram.png)
Copy link

Choose a reason for hiding this comment

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

Not sure if the sequence diagram should have additional objects at the footing.


#### `Parser`

![ParserClassUML](diagrams/ParserClassUML.png)
Copy link

Choose a reason for hiding this comment

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

It seems like the parser methods are static. Perhaps they should be underlined in the class diagram?

For example, validateAddPatient validates the parameter of `add patient` command, ensuring each parameter is in correct
format. Please refer to the below sequence diagram for a clearer understanding.

![ValidatorUML](diagrams/ValidatorUML.png)
Copy link

Choose a reason for hiding this comment

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

It seems like the validator methods are static as well. Perhaps they should be underlined in the class diagram?


### UI component

![UIClassUML](diagrams/UIClassUML.png)
Copy link

Choose a reason for hiding this comment

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

Consider following the lecture uml diagram notation for showing visibility and abstract classes.

Copy link

@FaliciaOng FaliciaOng left a comment

Choose a reason for hiding this comment

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

The developer guide was fairly clear. Good job!


The Sequence Diagram below showcases the general Logic and Flow of the program from Launch till Exit.

![Sequence Diagram](diagrams/SequenceDiagram.png)

Choose a reason for hiding this comment

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

image

Is this format correct? Should the lifeline stop after the deletion of an object?

For example, validateAddPatient validates the parameter of `add patient` command, ensuring each parameter is in correct
format. Please refer to the below sequence diagram for a clearer understanding.

![ValidatorUML](diagrams/ValidatorUML.png)

Choose a reason for hiding this comment

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

image

Should there be a self-invocation bar for each calling of own method?

shxr3f and others added 16 commits March 31, 2022 18:27
Changing saveString() method for patient and doctor
IdGenerator has a static method to generator a pseudo unique ID which
is a 8 digit number in String format. Can be used for other lists as
well. Still in the midst of changing the add appt cmd. Currently, it
should check if patient and doctor exists. However, still need to
implement adding into the appointment list.
In order to retrieve info regarding patient and doctor names to
simplify add appt command, apptList needs to know about patients
and doctors.
IdGenerator now has a method that is specific to add appt. New
add appt generates a pseudo unique hash from patient and doctor nric
and appt date.
So that appt data is correctly loaded upon start up.
Made error message for name error to be more explicit in rejecting
special characters and numbers.
Made error message for invalid age input to be more explicit in
referring to the age parameter.
Update UG DG to reflect the change that view doctor can now view by
specific criteria.
Update UG DG to reflect changes to find patient being able to search
by criteria.
Update UG DG to reflect changes to view medicine feature.
Anvitha-r and others added 30 commits April 11, 2022 19:05
Fixing issue with loading data of older date
* 'master' of https://github.com/AY2122S2-CS2113T-T09-3/tp:
  Fixing issue with loading data of older date
  Update wraineflores.md
  Inserting image into About Us
  Removing edit apppointment function and fixing delete appointment bugs
  Update wraineflores.md
  Update wraineflores.md
  add screenshots for all functions
  remove edit appointment
  Update pt ptList appt apptList UML Diag to reflect new methods added
  update UG with screenshot for add
  add pics for add function UG
  Reduce size of Manager UML image
  Update appt and apptList UML images to reflect new methods
  add line break
  crop picture
  fix findpatient exit error
  help-change
  add findappointment screenshot

# Conflicts:
#	docs/UserGuide.md
…into UG-update-changes

* 'master' of https://github.com/AY2122S2-CS2113T-T09-3/tp:
  Fixing issue with loading data of older date
* master:

# Conflicts:
#	docs/UserGuide.md
Add sharif photo in and one more UG section hyperlink
Hopefully this IS the final changes
Age must corresponse with birthdate and in this case should be 22
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.

10 participants