Refactor edit commands and remove dead code (#287)

# Changes - Instead of one long execute method for `edit_elderly` and `edit_volunteer` there is now 2 additional methods. - `edit` command calls the relevant static methods to prevent code duplication - `edit` command now gives warning for edited person has non matching date or region - remove dead code that I found Resolves #284
AY2223S2-CS2103T-W12-1 · Apr 9, 2023 · 6fc5900 · 6fc5900
2 parents 42d4700 + 8917c7b
commit 6fc5900
Show file tree

Hide file tree

Showing 5 changed files with 105 additions and 60 deletions.
diff --git a/src/main/java/seedu/address/logic/commands/EditCommand.java b/src/main/java/seedu/address/logic/commands/EditCommand.java
@@ -104,20 +104,19 @@ public CommandResult execute(Model model) throws CommandException {
 
     private CommandResult editElderly(Model model) throws CommandException {
         Elderly elderlyToEdit = model.getElderly(nric);
-        Elderly editedElderly = EditDescriptor.createEditedElderly(
-                elderlyToEdit, editDescriptor);
+        Elderly editedElderly = EditDescriptor.createEditedElderly(elderlyToEdit, editDescriptor);
         Nric editedNric = editedElderly.getNric();
+
         if (!elderlyToEdit.isSamePerson(editedElderly) && model.hasElderly(editedNric)) {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_ELDERLY);
         }
         if (model.hasVolunteer(editedNric)) {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_VOLUNTEERS);
         }
 
-        model.setElderly(elderlyToEdit, editedElderly);
-        model.refreshAllFilteredLists();
-        return new CommandResult(String.format(
-                EditElderlyCommand.MESSAGE_EDIT_ELDERLY_SUCCESS, editedElderly));
+        String finalMessage = EditElderlyCommand.updateElderly(
+                model, elderlyToEdit, editedElderly);
+        return new CommandResult(finalMessage);
     }
 
     private CommandResult editVolunteer(Model model) throws CommandException {
@@ -132,10 +131,9 @@ private CommandResult editVolunteer(Model model) throws CommandException {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_ELDERLY);
         }
 
-        model.setVolunteer(volunteerToEdit, editedVolunteer);
-        model.refreshAllFilteredLists();
-        return new CommandResult(String.format(
-                EditVolunteerCommand.MESSAGE_EDIT_VOLUNTEER_SUCCESS, editedVolunteer));
+        String finalMessage = EditVolunteerCommand.updateVolunteer(
+                model, volunteerToEdit, editedVolunteer);
+        return new CommandResult(finalMessage);
     }
 
     @Override

diff --git a/src/main/java/seedu/address/logic/commands/EditElderlyCommand.java b/src/main/java/seedu/address/logic/commands/EditElderlyCommand.java
@@ -92,49 +92,74 @@ public EditElderlyCommand(Index index, EditDescriptor editDescriptor) {
         this.editDescriptor = new EditDescriptor(editDescriptor);
     }
 
-    @Override
-    public CommandResult execute(Model model) throws CommandException {
-        if (!editDescriptor.isAnyFieldEdited()) {
-            throw new CommandException(MESSAGE_NO_FIELD_PROVIDED);
+    /**
+     * Replaces the elderly that has been edited with the edited elderly.
+     *
+     * @param model FriendlyLink's model.
+     * @param elderlyToEdit Elderly to replace.
+     * @param editedElderly Elderly with new fields.
+     * @return Command result message.
+     */
+    public static String updateElderly(Model model, Elderly elderlyToEdit, Elderly editedElderly) {
+        model.setElderly(elderlyToEdit, editedElderly);
+
+        @SuppressWarnings("unchecked")
+        Predicate<Elderly> predicate = (Predicate<Elderly>) PREDICATE_SHOW_ALL;
+        model.updateFilteredElderlyList(predicate);
+        String finalMessage = String.format(MESSAGE_EDIT_ELDERLY_SUCCESS, editedElderly);
+
+        if (!model.check(editedElderly, Person::isSuitableRegion)) {
+            finalMessage += MESSAGE_WARNING_REGION;
         }
+        if (!model.check(editedElderly, Person::hasSuitableAvailableDates)) {
+            finalMessage += MESSAGE_WARNING_AVAILABLE_DATES;
+        }
+
+        model.refreshAllFilteredLists();
+        return finalMessage;
+    }
 
+    @Override
+    public CommandResult execute(Model model) throws CommandException {
         requireNonNull(model);
-        List<Elderly> lastShownList = model.getFilteredElderlyList();
 
-        if (index.getZeroBased() >= lastShownList.size()) {
-            throw new CommandException(MESSAGE_INVALID_ELDERLY_DISPLAYED_INDEX);
+        if (!editDescriptor.isAnyFieldEdited()) {
+            throw new CommandException(MESSAGE_NO_FIELD_PROVIDED);
         }
-        assert index.getZeroBased() >= 0 : "index should not be negative";
 
-        Elderly elderlyToEdit = lastShownList.get(index.getZeroBased());
+        Elderly elderlyToEdit = getElderlyToEdit(model);
         Elderly editedElderly = EditDescriptor.createEditedElderly(elderlyToEdit, editDescriptor);
-
         Nric editedNric = editedElderly.getNric();
+
         if (!elderlyToEdit.isSamePerson(editedElderly) && model.hasElderly(editedNric)) {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_ELDERLY);
         }
         if (model.hasVolunteer(editedNric)) {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_VOLUNTEERS);
         }
 
-        model.setElderly(elderlyToEdit, editedElderly);
-        @SuppressWarnings("unchecked")
-        Predicate<Elderly> predicate = (Predicate<Elderly>) PREDICATE_SHOW_ALL;
-        model.updateFilteredElderlyList(predicate);
+        String finalMessage = updateElderly(model, elderlyToEdit, editedElderly);
+        return new CommandResult(finalMessage);
+    }
 
-        String finalMessage = String.format(MESSAGE_EDIT_ELDERLY_SUCCESS, editedElderly);
-        if (!model.check(editedElderly, Person::isSuitableRegion)) {
-            finalMessage += MESSAGE_WARNING_REGION;
-        }
-        if (!model.check(editedElderly, Person::hasSuitableAvailableDates)) {
-            finalMessage += MESSAGE_WARNING_AVAILABLE_DATES;
+    /**
+     * Returns the elderly that has been edited.
+     *
+     * @param model FriendlyLink's model.
+     * @return Elderly that has been edited.
+     * @throws CommandException If index is invalid.
+     */
+    private Elderly getElderlyToEdit(Model model) throws CommandException {
+        List<Elderly> lastShownList = model.getFilteredElderlyList();
+
+        if (index.getZeroBased() >= lastShownList.size()) {
+            throw new CommandException(MESSAGE_INVALID_ELDERLY_DISPLAYED_INDEX);
         }
+        assert index.getZeroBased() >= 0 : "index should not be negative";
 
-        model.refreshAllFilteredLists();
-        return new CommandResult(finalMessage);
+        return lastShownList.get(index.getZeroBased());
     }
 
-
     @Override
     public boolean equals(Object other) {
         // short circuit if same object

diff --git a/src/main/java/seedu/address/logic/commands/EditVolunteerCommand.java b/src/main/java/seedu/address/logic/commands/EditVolunteerCommand.java
@@ -92,21 +92,42 @@ public EditVolunteerCommand(Index index, EditDescriptor editDescriptor) {
         this.editDescriptor = new EditDescriptor(editDescriptor);
     }
 
-    @Override
-    public CommandResult execute(Model model) throws CommandException {
-        if (!editDescriptor.isAnyFieldEdited()) {
-            throw new CommandException(MESSAGE_NO_FIELD_PROVIDED);
+    /**
+     * Replaces the volunteer that has been edited with the edited volunteer.
+     *
+     * @param model FriendlyLink's model.
+     * @param volunteerToEdit Volunteer to replace.
+     * @param editedVolunteer Volunteer with new fields.
+     * @return Command result message.
+     */
+    public static String updateVolunteer(Model model, Volunteer volunteerToEdit, Volunteer editedVolunteer) {
+        model.setVolunteer(volunteerToEdit, editedVolunteer);
+
+        @SuppressWarnings("unchecked")
+        Predicate<Volunteer> predicate = (Predicate<Volunteer>) PREDICATE_SHOW_ALL;
+        model.updateFilteredVolunteerList(predicate);
+        String finalMessage = String.format(MESSAGE_EDIT_VOLUNTEER_SUCCESS, editedVolunteer);
+
+        if (!model.check(editedVolunteer, Person::isSuitableRegion)) {
+            finalMessage += MESSAGE_WARNING_REGION;
+        }
+        if (!model.check(editedVolunteer, Person::hasSuitableAvailableDates)) {
+            finalMessage += MESSAGE_WARNING_AVAILABLE_DATES;
         }
 
+        model.refreshAllFilteredLists();
+        return finalMessage;
+    }
+
+    @Override
+    public CommandResult execute(Model model) throws CommandException {
         requireNonNull(model);
-        List<Volunteer> lastShownList = model.getFilteredVolunteerList();
 
-        if (index.getZeroBased() >= lastShownList.size()) {
-            throw new CommandException(MESSAGE_INVALID_VOLUNTEER_DISPLAYED_INDEX);
+        if (!editDescriptor.isAnyFieldEdited()) {
+            throw new CommandException(MESSAGE_NO_FIELD_PROVIDED);
         }
-        assert index.getZeroBased() >= 0 : "index should not be negative";
 
-        Volunteer volunteerToEdit = lastShownList.get(index.getZeroBased());
+        Volunteer volunteerToEdit = getVolunteerToEdit(model);
         Volunteer editedVolunteer = EditDescriptor.createEditedVolunteer(
                 volunteerToEdit, editDescriptor);
 
@@ -118,21 +139,26 @@ public CommandResult execute(Model model) throws CommandException {
             throw new CommandException(MESSAGE_DUPLICATE_PERSON_IN_ELDERLY);
         }
 
-        model.setVolunteer(volunteerToEdit, editedVolunteer);
-        @SuppressWarnings("unchecked")
-        Predicate<Volunteer> predicate = (Predicate<Volunteer>) PREDICATE_SHOW_ALL;
-        model.updateFilteredVolunteerList(predicate);
+        String finalMessage = updateVolunteer(model, volunteerToEdit, editedVolunteer);
+        return new CommandResult(finalMessage);
+    }
 
-        String finalMessage = String.format(MESSAGE_EDIT_VOLUNTEER_SUCCESS, editedVolunteer);
-        if (!model.check(editedVolunteer, Person::isSuitableRegion)) {
-            finalMessage += MESSAGE_WARNING_REGION;
-        }
-        if (!model.check(editedVolunteer, Person::hasSuitableAvailableDates)) {
-            finalMessage += MESSAGE_WARNING_AVAILABLE_DATES;
+    /**
+     * Returns the volunteer that has been edited.
+     *
+     * @param model FriendlyLink's model.
+     * @return Volunteer that has been edited.
+     * @throws CommandException If index is invalid.
+     */
+    private Volunteer getVolunteerToEdit(Model model) throws CommandException {
+        List<Volunteer> lastShownList = model.getFilteredVolunteerList();
+
+        if (index.getZeroBased() >= lastShownList.size()) {
+            throw new CommandException(MESSAGE_INVALID_VOLUNTEER_DISPLAYED_INDEX);
         }
+        assert index.getZeroBased() >= 0 : "index should not be negative";
 
-        model.refreshAllFilteredLists();
-        return new CommandResult(finalMessage);
+        return lastShownList.get(index.getZeroBased());
     }
 
     @Override

diff --git a/src/test/java/seedu/address/logic/commands/EditCommandTest.java b/src/test/java/seedu/address/logic/commands/EditCommandTest.java
@@ -6,6 +6,7 @@
 import static seedu.address.commons.core.Messages.MESSAGE_DUPLICATE_PERSON_IN_VOLUNTEERS;
 import static seedu.address.commons.core.Messages.MESSAGE_NO_FIELD_PROVIDED;
 import static seedu.address.commons.core.Messages.MESSAGE_NRIC_NOT_EXIST;
+import static seedu.address.commons.core.Messages.MESSAGE_WARNING_REGION;
 import static seedu.address.logic.commands.CommandTestUtil.DESC_AMY;
 import static seedu.address.logic.commands.CommandTestUtil.DESC_BOB;
 import static seedu.address.logic.commands.CommandTestUtil.VALID_NRIC_AMY;
@@ -42,7 +43,7 @@ public void execute_validElderlyNric_success() {
         EditCommand editCommand = new EditCommand(selectedNric, descriptor);
 
         String expectedMessage = String.format(EditElderlyCommand.MESSAGE_EDIT_ELDERLY_SUCCESS,
-                resultantElderly);
+                resultantElderly) + MESSAGE_WARNING_REGION;
 
         Model expectedModel = new ModelManagerBuilder()
                 .withFriendlyLink(model.getFriendlyLink())

diff --git a/src/test/java/seedu/address/model/person/information/RiskLevelTest.java b/src/test/java/seedu/address/model/person/information/RiskLevelTest.java
@@ -9,11 +9,6 @@
 
 class RiskLevelTest {
 
-    //    @Test // RiskLevel does not allow RiskLevel(null) constructor
-    //    public void constructor_null_throwsNullPointerException() {
-    //        assertThrows(NullPointerException.class, () -> new RiskLevel(null));
-    //    }
-
     @Test
     public void constructor_invalidRisk_throwsIllegalArgumentException() {
         String invalidRisk = "abc";