Add tests for Add, Edit and Delete commands and optimise code (#115)

* v1.1

* fix some errors and typos

* Refactor entire project, remove all traces of Addressbook

* Update developer guide for ExportCommandP (#80)

* Update documentation for view command (#81)

* Remove remaining Addressbook classes

* Refactor some classes

* fix missing files issue

* Add test cases for add function, Utils and other code enhancements

* fix codacy issues

* fix codacy issues

* Add tests for Utils

* Add diagrams

* Update Ui.png

* fix issue with Ui.png

* Update Codacy Badge link due to reinitialization.

* Enhance ordering of slots printed

* Update UserGuide.adoc with feedback from peers

* Update UserGuide.adoc with nicer tables

* Update UserGuide.adoc: fix formatting issues

* UserGuide.adoc Remove potential Netlify breaking code

* Add tests for Add, Edit and Delete commands and optimise code

* Fix checkstyle error

src/planmysem/commands/AddCommand.java

@@ -31,7 +31,6 @@ public class AddCommand extends Command {
             + " n/CS2113T Tutorial d/mon st/08:00 et/09:00 des/Topic: Sequence Diagram t/CS2113T "
             + "t/Tutorial r/normal";
 
-    public static final String MESSAGE_SUCCESS_NO_CHANGE = "No slots were added.";
     public static final String MESSAGE_SUCCESS = "%1$s Slots added.\n\n%2$s";
     public static final String MESSAGE_FAIL_OUT_OF_BOUNDS = "Date specified is out of bounds.";
 
@@ -75,12 +74,8 @@ public CommandResult execute() {
             }
         }
 
-        if (dates.size() == 0) {
-            return new CommandResult(MESSAGE_SUCCESS_NO_CHANGE);
-        } else {
-            return new CommandResult(String.format(MESSAGE_SUCCESS, dates.size(),
-                    craftSuccessMessage(days, slot)));
-        }
+        return new CommandResult(String.format(MESSAGE_SUCCESS, dates.size(),
+                craftSuccessMessage(days, slot)));
     }
 
     /**

src/planmysem/commands/Command.java

@@ -3,19 +3,20 @@
 import static planmysem.ui.Gui.DISPLAYED_INDEX_OFFSET;
 
 import java.time.LocalDate;
-import java.util.List;
+import java.util.Map;
 
 import javafx.util.Pair;
 import planmysem.common.Messages;
 import planmysem.data.Planner;
+import planmysem.data.semester.ReadOnlyDay;
 import planmysem.data.slot.ReadOnlySlot;
 
 /**
  * Represents an executable command.
  */
 public abstract class Command {
     protected Planner planner;
-    protected List<Pair<LocalDate, ? extends ReadOnlySlot>> relevantSlots;
+    protected Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> relevantSlots;
     private int targetIndex = -1;
 
     /**
@@ -46,14 +47,15 @@ protected Command() {
      * @param slots used to generate summary
      * @return summary message for persons displayed
      */
-    public static String getMessageForSlotsListShownSummary(List<Pair<LocalDate, ? extends ReadOnlySlot>> slots) {
+    public static String getMessageForSlotsListShownSummary(Map<LocalDate,
+            Pair<ReadOnlyDay, ReadOnlySlot>> slots) {
         return String.format(Messages.MESSAGE_PERSONS_LISTED_OVERVIEW, slots.size());
     }
 
     /**
      * Supplies the data the command will operate on.
      */
-    public void setData(Planner planner, List<Pair<LocalDate, ? extends ReadOnlySlot>> slots) {
+    public void setData(Planner planner, Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> slots) {
         this.planner = planner;
         this.relevantSlots = slots;
     }
@@ -62,25 +64,33 @@ public int getTargetIndex() {
         return targetIndex;
     }
 
-    public void setTargetIndex(int targetIndex) {
-        this.targetIndex = targetIndex;
-    }
+    //    public void setTargetIndex(int targetIndex) {
+    //        this.targetIndex = targetIndex;
+    //    }
 
 
     /**
      * Extracts the the target Day in the last shown list from the given arguments.
      *
      * @throws IndexOutOfBoundsException if the target index is out of bounds of the last viewed listing
      */
-    protected List<Pair<LocalDate, ? extends ReadOnlySlot>> getTargetSlots() throws IndexOutOfBoundsException {
-        return relevantSlots;
-    }
+    //    protected Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> getTargetSlots() throws IndexOutOfBoundsException {
+    //        return relevantSlots;
+    //    }
 
-    protected Pair<LocalDate, ? extends ReadOnlySlot> getTargetSlot(int index) throws IndexOutOfBoundsException {
-        return relevantSlots.get(index);
-    }
+    protected Pair<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> getTargetSlot() throws IndexOutOfBoundsException {
+        if (relevantSlots == null || relevantSlots.size() < targetIndex) {
+            throw new IndexOutOfBoundsException();
+        }
+
+        int count = 0;
+        for (Map.Entry<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> entry : relevantSlots.entrySet()) {
+            if (count == targetIndex - DISPLAYED_INDEX_OFFSET) {
+                return new Pair(entry.getKey(), entry.getValue());
+            }
+            count++;
+        }
 
-    protected Pair<LocalDate, ? extends ReadOnlySlot> getTargetSlot() throws IndexOutOfBoundsException {
-        return relevantSlots.get(targetIndex - DISPLAYED_INDEX_OFFSET);
+        throw new IndexOutOfBoundsException();
     }
 }

src/planmysem/commands/CommandResult.java

@@ -1,10 +1,11 @@
 package planmysem.commands;
 
 import java.time.LocalDate;
-import java.util.List;
+import java.util.Map;
 import java.util.Optional;
 
 import javafx.util.Pair;
+import planmysem.data.semester.ReadOnlyDay;
 import planmysem.data.slot.ReadOnlySlot;
 
 /**
@@ -20,22 +21,22 @@ public class CommandResult {
     /**
      * The list of Slots that was produced by the command
      */
-    private final List<Pair<LocalDate, ? extends ReadOnlySlot>> slots;
+    private final Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> slots;
 
     public CommandResult(String feedbackToUser) {
         this.feedbackToUser = feedbackToUser;
         slots = null;
     }
 
-    public CommandResult(String feedbackToUser, List<Pair<LocalDate, ? extends ReadOnlySlot>> slots) {
+    public CommandResult(String feedbackToUser, Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> slots) {
         this.feedbackToUser = feedbackToUser;
         this.slots = slots;
     }
 
     /**
      * Returns list of Slots relevant to the command command result, if any.
      */
-    public Optional<List<Pair<LocalDate, ? extends ReadOnlySlot>>> getRelevantSlots() {
+    public Optional<Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>>> getRelevantSlots() {
         return Optional.ofNullable(slots);
     }
 

src/planmysem/commands/DeleteCommand.java

@@ -1,7 +1,6 @@
 package planmysem.commands;
 
 import java.time.LocalDate;
-import java.time.LocalDateTime;
 import java.util.HashSet;
 import java.util.Map;
 import java.util.Set;
@@ -11,9 +10,8 @@
 import planmysem.common.Messages;
 import planmysem.common.Utils;
 import planmysem.data.exception.IllegalValueException;
-import planmysem.data.semester.Day;
+import planmysem.data.semester.ReadOnlyDay;
 import planmysem.data.slot.ReadOnlySlot;
-import planmysem.data.slot.Slot;
 import planmysem.data.tag.Tag;
 
 /**
@@ -33,11 +31,8 @@ public class DeleteCommand extends Command {
             + "\n\tExample 2: " + COMMAND_WORD
             + " 2";
 
-
     public static final String MESSAGE_SUCCESS_NO_CHANGE = "No Slots were deleted.\n\n%1$s";
     public static final String MESSAGE_SUCCESS = "%1$s Slots deleted.\n\n%2$s\n%3$s";
-    public static final String MESSAGE_FAIL_ILLEGAL_VALUE = MESSAGE_SUCCESS_NO_CHANGE
-            + " Illegal characters were detected.";
 
     private final Set<Tag> tags = new HashSet<>();
 
@@ -59,70 +54,30 @@ public DeleteCommand(int index) {
 
     @Override
     public CommandResult execute() {
-        Map<LocalDateTime, ReadOnlySlot> selectedSlots = new TreeMap<>();
+        Map<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> selectedSlots = new TreeMap<>();
 
         if (getTargetIndex() == -1) {
-            for (Map.Entry<LocalDate, Day> day : planner.getSemester().getDays().entrySet()) {
-                for (Slot slot : day.getValue().getSlots()) {
-                    if (slot.getTags().containsAll(tags)) {
-                        selectedSlots.put(LocalDateTime.of(day.getKey(), slot.getStartTime()), slot);
-                    }
-                }
-            }
+            selectedSlots.putAll(planner.getSlots(tags));
+
             if (selectedSlots.size() == 0) {
                 return new CommandResult(String.format(MESSAGE_SUCCESS_NO_CHANGE,
                         Messages.craftSelectedMessage(tags)));
             }
         } else {
             try {
-                final Pair<LocalDate, ? extends ReadOnlySlot> target = getTargetSlot();
-                selectedSlots.put(LocalDateTime.of(target.getKey(),
-                        target.getValue().getStartTime()), target.getValue());
-
-                if (!planner.containsSlot(target.getKey(), target.getValue())) {
-                    return new CommandResult(Messages.MESSAGE_SLOT_NOT_IN_PLANNER);
-                }
+                final Pair<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> target = getTargetSlot();
+                selectedSlots.put(target.getKey(), target.getValue());
             } catch (IndexOutOfBoundsException ie) {
                 return new CommandResult(Messages.MESSAGE_INVALID_SLOT_DISPLAYED_INDEX);
             }
         }
 
         // perform deletion of slots from the planner
-        for (Map.Entry<LocalDateTime, ? extends ReadOnlySlot> slot: selectedSlots.entrySet()) {
-            planner.getSemester().removeSlot(slot.getKey().toLocalDate(), slot.getValue());
+        for (Map.Entry<LocalDate, Pair<ReadOnlyDay, ReadOnlySlot>> entry: selectedSlots.entrySet()) {
+            planner.removeSlot(entry.getKey(), entry.getValue().getValue());
         }
 
         return new CommandResult(String.format(MESSAGE_SUCCESS, selectedSlots.size(),
-                Messages.craftSelectedMessage(tags), craftSuccessMessage(selectedSlots)));
+                Messages.craftSelectedMessage(tags), Messages.craftSelectedMessage("Deleted Slots:", selectedSlots)));
     }
-
-    /**
-     * Craft success message.
-     */
-    private String craftSuccessMessage(Map<LocalDateTime, ReadOnlySlot> selectedSlots) {
-        StringBuilder sb = new StringBuilder();
-
-        sb.append("Deleted Slots: ");
-        sb.append("\n");
-
-        int count = 1;
-        for (Map.Entry<LocalDateTime, ReadOnlySlot> editedSlot : selectedSlots.entrySet()) {
-            sb.append(count);
-            sb.append(".\t");
-            sb.append(editedSlot.getValue().getName().toString());
-            sb.append(", ");
-            sb.append(editedSlot.getKey().toLocalDate().toString());
-            sb.append(" ");
-            sb.append(editedSlot.getKey().toLocalTime().toString());
-            sb.append(", ");
-            sb.append(planner.getSemester().getDays().get(editedSlot.getKey().toLocalDate()).getType());
-            sb.append(", ");
-            sb.append(editedSlot.getKey().getDayOfWeek().toString());
-            count++;
-            sb.append("\n");
-        }
-
-        return sb.toString();
-    }
-
 }