Modify Delete Command to use IC Number #120
Conversation
The existing delete command uses the index to delete a patient from the list. This can be very cumbersome especially in very large datasets of patients. Hence, use IC Number of the patient instead to delete a patient directly from the list like "delete T1234567B" Let's update the Delete Command and fix tests for the DeleteCommand and Parser tests.
The existing edit command uses the index to edit a patient's description in the list. This can be very cumbersome especially in very large datasets of patients. Hence, use IC Number of the patient instead to edit a patient's description directly from the list like "edit T1234567B n/NEW NAME" Let's update the EDIT Command and fix tests for the EditCommand and Parser tests.
Making changes to add prefix i/ in front of the IC Numbers for these commands to standardize the command format as per the User Guide
Making changes to add prefix i/ in front of the IC Numbers for these commands to standardize the command format as per the User Guide for the Delete Command
| @@ -24,23 +24,18 @@ public class DeleteCommand extends Command { | |||
| + "Example: " + COMMAND_WORD + " 1"; | |||
There was a problem hiding this comment.
Hi, since the implementation of delete command is by ic number, would it be good to also update the message usage accordingly?
|
|
||
| /** | ||
| * Parses the given {@code String} of arguments in the context of the DeleteCommand | ||
| * and returns a DeleteCommand object for execution. | ||
| * @throws ParseException if the user input does not conform the expected format | ||
| */ | ||
| public DeleteCommand parse(String args) throws ParseException { | ||
| ArgumentMultimap argMultimap = ArgumentTokenizer.tokenize(args, REQUIRED_PREFIXES); |
There was a problem hiding this comment.
I like how you followed the implementation for addcommandparser to update this new DeleteCommandParser, good job!!
| Index index = ParserUtil.parseIndex(args); | ||
| return new DeleteCommand(index); | ||
| IcNumber icNumber = ParserUtil.parseIcNumber(argMultimap.getValue(PREFIX_IC_NUMBER).get()); | ||
| //IcNumber icNumber = ParserUtil.parseIcNumber(args); |
There was a problem hiding this comment.
If this is no longer needed, would it be good to remove it?
| public static void showPatientAtIC(Model model, IcNumber icNumber) { | ||
| List<Patient> lastShownList = model.getFilteredPatientList(); | ||
| Patient patient = model.getPatient(icNumber, lastShownList); | ||
| final String[] splitName = patient.getName().fullName.split("\\s+"); | ||
| model.updateFilteredPatientList(new NameContainsKeywordsPredicate(Arrays.asList(splitName[0]))); | ||
| assertEquals(1, model.getFilteredPatientList().size()); | ||
| } | ||
|
|
There was a problem hiding this comment.
I like how you tried to implement a testing utility to show the patient of a IC. But considering how you are using the filtered patient list, would it be good if we can use the full list instead? since filtered patient list does not contain all patients
There was a problem hiding this comment.
Nice job updating the EditCommandParserTest!! would be good to find ways to update the testing utility since as per the user guide the format for command is edit i/ number.
There was a problem hiding this comment.
Yup, that could be our focus point in the next iteration!
| @Override | ||
| public Patient getPatient(IcNumber icNumber, List<Patient> patientList){ | ||
| for (int i = 0; i < patientList.size(); i++) { | ||
| if(patientList.get(i).getIcNumber().equals(icNumber)) | ||
| return patientList.get(i); | ||
| } | ||
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
I like how you implement your getPatient!! But if the patient could not be found, it could be quite dangerous to return a null. Do ensure that the other party calling this function checks for null. If it does receive a null, would be good to throw an exception!!! Could see my implementation of the exception PatientWithFieldNotFoundException. It has yet to be merged in code base but would be good if we could make use of this exception!!
| /*public Patient getPatient(IcNumber icNumber, List<Patient> patientList){ | ||
| for (int i = 0; i < patientList.size(); i++) { | ||
| if(patientList.get(i).getIcNumber().equals(icNumber)) | ||
| return patientList.get(i); | ||
| } | ||
| return null; | ||
| }*/ | ||
|
|
There was a problem hiding this comment.
Would be good if we remove unused code!!
The existing delete command uses the index to delete a patient from the list. This can be very cumbersome especially in very large datasets of patients.
Hence, use IC Number of the patient instead to delete a patient directly from the list like "delete T1234567B"
Let's update the Delete Command and fix tests for the DeleteCommand and Parser tests.