-
Notifications
You must be signed in to change notification settings - Fork 5
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
Branch update patient fields #102
Branch update patient fields #102
Conversation
Currently, Patient class is taking in Department and Record as input Department and Record are to be created when Patient is created, thus it shouldnt be an input Update Patient constructor to remove AssignedDepartment and Record parameters, also initialiazed them with default values
Currently, the instantiation of Record class does not initialise default values for other fields. This means that if a particular program wants to get its attribute value, it could lead to null value. Update Record.java to include 3 default initialization values for its fields
Currently, the sample patients are using outdated fields. It is important to update the patients to use the new fields. Add the Gender, IcNumber and Birthday fields to sample Patients
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general! However, I have made some suggestions and highlighted some potential issues that would be good to look into :)
@@ -44,8 +44,8 @@ public Patient(Name name, Phone phone, Email email, Gender gender, IcNumber icNu | |||
this.birthday = birthday; | |||
this.address = address; | |||
this.tags.addAll(tags); | |||
this.assignedDepartment = assignedDepartment; | |||
this.record = record; | |||
this.assignedDepartment = new AssignedDepartment(); // default Department given |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Just to note that we might be changing this such that the Patient is immediately assigned to the Emergency Department upon creation so that it is reflected in the database straight away and the doctor won't have to manually assign a newly admitted patient to the emergency department. The default department is displayed as "-" as a string and is meant to be used for inactive patients, but for now we can just assign it to newly instantiated patient objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, have taken note of this, thanks!
.add("treatmentPlan", treatmentPlan) | ||
.toString(); | ||
return new ToStringBuilder(this).add("patient", patient).add("initialObservations", initialObservations) | ||
.add("diagnosis", diagnosis).add("treatmentPlan", treatmentPlan).toString(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about the indentation here but I feel like the one before looks a bit neater? 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would fix the indentation one shot after further changes, thanks for pointing out!
import seedu.address.model.patient.Name; | ||
import seedu.address.model.patient.Patient; | ||
import seedu.address.model.patient.Phone; | ||
import seedu.address.model.patient.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I believe that the coding standard guide says that we should list the imports explicitly instead of using wildcard imports? If not then it's ok
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, java have the tendency to change all related imports to a single *, will update it!!
@JsonProperty("tags") List<JsonAdaptedTag> tags) { | ||
this.name = name; | ||
this.phone = phone; | ||
this.email = email; | ||
this.gender = gender; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm it appears that the patient's department is not stored here. I feel like it might be needed though, in the case where for whatever reason the user has to relaunch the app (e.g. in the middle of a shift) and finds that all the data about the patient's department is gone (then they wouldn't know where each patient is now).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking we can overload the constructor and have two constructors. One can be for the reading of addressbook from file (need to store department info) and we can have the constructor take in a department string as an argument. The other is for creating new patient objects (like for the add command), then we don't need the department param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also we need to eventually find a way to store the records into the hard disk as well 😔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, i think i will create a github task for the next iteration. Definitely will have to focus on storage next iteration!!
@@ -11,6 +11,7 @@ public class Age { | |||
public static final String MESSAGE_CONSTRAINTS = | |||
"Age should only contain numbers, and it should not be negative"; | |||
public static final String VALIDATION_REGEX = "\\d+"; | |||
public static String DEFAULT_AGE = "30"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, perhaps it will be better for the dummy default values to be more obvious (like '00') or something? So that the user will less likely mix up the entries with non-default values and those with default values (in case they want to come back and fill in the details again). This applies to default birthday and other attributes as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out. Have updated the values to look more default!
public static Patient createPatientFromPresentPrefixes(Name name, Phone phone, Email email, Gender gender, | ||
IcNumber icNumber, Birthday birthday, Address address, | ||
Set<Tag> tags, ArgumentMultimap argMultimap, | ||
Prefix[] prefixes) throws ParseException { | ||
for (Prefix p : prefixes) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also I'm not sure whether I am tripping or missing something but I don't see the cases for the other prefixes here? 🤣 Or will they only be added later?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup they will be added later!! Would require the ParserUtil class to be completed before i can continue the implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup everything looks good! Definitely agree with the changes Jingya suggested! Points of focus for the next iteration:
- Including the remaining prefixes to be parsed to create patient objects
- Finding a way to load and save tasks for storage of patient info.
- Updating the Emergency Department where - is used for inactive patients rather than new patients!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for making these changes! The default values are a lot clearer now :)
Summary of change
Fixed the Patient class constructor and updated some of its dependency to use the new fields. More details on dependency given below.
Key changes
Update
Patient.java
constructor1.Update the constructor parameter to take in correct arguments.
Update
Record.java
constructor1.Update Record object to initialise with default values for its fields.
Update
PatientBuilder.java
1.Update PatientBuilder class to use new Patient fields
Update Patient attribute fields
1.Update Patient attribute fields to have a default values
Update
AddCommandParser.java
class1.Update AddCommandParser.java to use new Patient attributes
Update
SampleDataUtil.java
1.Update the Patient sample data in SampleDataUtil.java to use the new Patient attributes
Update JsonAdaptedPatient files
1.Update
JsonAdaptedPatient.java
to use the new Patient fields.2.Update
JsonAdaptedPatientTest.java
to use the new Patient fields.