-
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
Add appointment feature #42
Conversation
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.
LGTM, nice implementation just make sure checkstyle is adhered to before merging
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.
neat 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.
thanks for the detailed comments, code is easily understandable
/** | ||
* Returns true if a given string is a valid appointment date. | ||
*/ | ||
public static boolean isValidAppointment(String test) { |
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.
maybe this method can be in the appropriate parser instead?
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.
Agreed, moved to parser. thanks
@@ -94,13 +104,14 @@ public boolean equals(Object other) { | |||
&& phone.equals(otherPerson.phone) | |||
&& email.equals(otherPerson.email) | |||
&& address.equals(otherPerson.address) | |||
&& tags.equals(otherPerson.tags); | |||
&& tags.equals(otherPerson.tags) | |||
&& Objects.equals(appointment, otherPerson.appointment); // Include appointment in equality check |
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.
could appointment.equals(otherPerson.appointment) work? think narrowing the scope might be safer
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.
Currently, appointment is deemed as an optional field, as not all patients may have an appointment.
For patients without appointments, their appointment variable holds a null value. With this implementation, appointment.equals(otherPerson.appointment)
will trigger a NullPointerException if appointment
is null. Using Object.equals() helps to prevent that.
I am also not too sure if this is the best way to go about doing it, can discuss?
Fixes #41