Merge pull request #477 from einride/master

enhancements in TourActivities
graphhopper · Sep 12, 2019 · 74c3c68 · 74c3c68
2 parents da0bff3 + 8076d46
commit 74c3c68
Show file tree

Hide file tree

Showing 4 changed files with 58 additions and 17 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -4,6 +4,7 @@ matrix:
   fast_finish: true
   include:
     - jdk: oraclejdk8
+      dist: trusty
       # Java 9 needs to be manually installed/upgraded
       # see: https://github.com/travis-ci/travis-ci/issues/2968#issuecomment-149164058
     - jdk: oraclejdk9

diff --git a/...it-core/src/main/java/com/graphhopper/jsprit/core/algorithm/recreate/BreakScheduling.java b/...it-core/src/main/java/com/graphhopper/jsprit/core/algorithm/recreate/BreakScheduling.java
@@ -45,8 +45,6 @@ public class BreakScheduling implements InsertionStartsListener,JobInsertedListe
 
     private final EventListeners eventListeners;
 
-    private Set<VehicleRoute> modifiedRoutes = new HashSet<VehicleRoute>();
-
     public BreakScheduling(VehicleRoutingProblem vrp, StateManager stateManager, ConstraintManager constraintManager) {
         this.stateManager = stateManager;
         this.breakInsertionCalculator = new BreakInsertionCalculator(vrp.getTransportCosts(), vrp.getActivityCosts(), new LocalActivityInsertionCostsCalculator(vrp.getTransportCosts(), vrp.getActivityCosts(), stateManager), constraintManager, vrp.getJobActivityFactory());
@@ -78,7 +76,6 @@ public void informJobInserted(Job job2insert, VehicleRoute inRoute, double addit
 
     @Override
     public void ruinStarts(Collection<VehicleRoute> routes) {
-
     }
 
     @Override
@@ -88,7 +85,7 @@ public void ruinEnds(Collection<VehicleRoute> routes, Collection<Job> unassigned
             boolean removed = route.getTourActivities().removeJob(aBreak);
             if(removed) logger.trace("ruin: {}", aBreak.getId());
         }
-        List<Break> breaks = new ArrayList<Break>();
+        List<Break> breaks = new ArrayList<>();
         for (Job j : unassignedJobs) {
             if (j instanceof Break) {
                 breaks.add((Break) j);
@@ -99,7 +96,6 @@ public void ruinEnds(Collection<VehicleRoute> routes, Collection<Job> unassigned
 
     @Override
     public void removed(Job job, VehicleRoute fromRoute) {
-        if(fromRoute.getVehicle().getBreak() != null) modifiedRoutes.add(fromRoute);
     }
 
     @Override
@@ -119,6 +115,5 @@ public void informInsertionStarts(Collection<VehicleRoute> vehicleRoutes, Collec
                 }
             }
         }
-
     }
 }
diff --git a/...main/java/com/graphhopper/jsprit/core/problem/solution/route/activity/TourActivities.java b/...main/java/com/graphhopper/jsprit/core/problem/solution/route/activity/TourActivities.java
@@ -81,15 +81,37 @@ private TourActivities(TourActivities tour2copy) {
     }
 
     public TourActivities() {
-
     }
 
     public List<TourActivity> getActivities() {
         return Collections.unmodifiableList(tourActivities);
     }
 
     public Iterator<TourActivity> iterator() {
-        return tourActivities.iterator();
+        final Iterator<TourActivity> iterator = tourActivities.iterator();
+        return new Iterator<TourActivity>() {
+            private TourActivity lastReturned = null;
+
+            @Override
+            public boolean hasNext() {
+                return iterator.hasNext();
+            }
+
+            @Override
+            public TourActivity next() {
+                return lastReturned = iterator.next();
+            }
+
+            @Override
+            public void remove() {
+                if (lastReturned instanceof JobActivity) {
+                    throw new IllegalStateException("Cannot remove JobActivities via iterator. "
+                        + "Use TourActivities.removeActivity(), or alternatively, consider TourActivities.removeJob()");
+                } else {
+                    iterator.remove();
+                }
+            }
+        };
     }
 
     public boolean isEmpty() {
@@ -154,26 +176,35 @@ public boolean removeJob(Job job) {
      * @return true if activity has been removed, false otherwise
      */
     public boolean removeActivity(TourActivity activity) {
-        Job job = null;
-        if (activity instanceof JobActivity) {
-            job = ((JobActivity) activity).getJob();
+        if (!(activity instanceof JobActivity)) {
+            //assumes that an activity can be added only once to tourActivities
+            return tourActivities.remove(activity);
         }
+
+        Job job = ((JobActivity) activity).getJob();
         boolean jobIsAlsoAssociateToOtherActs = false;
         boolean actRemoved = false;
-        List<TourActivity> acts = new ArrayList<>(tourActivities);
-        for (TourActivity act : acts) {
+        for (TourActivity act : new ArrayList<>(tourActivities)) {
             if (act == activity) {
                 tourActivities.remove(act);
+                if (jobIsAlsoAssociateToOtherActs) {
+                    // other activities also refer to job --> do not remove job
+                    // thus no need to iterate any further
+                    return true;
+                }
                 actRemoved = true;
             } else {
-                if (act instanceof JobActivity && job != null) {
-                    if (((JobActivity) act).getJob().equals(job)) {
-                        jobIsAlsoAssociateToOtherActs = true;
+                if (act instanceof JobActivity && ((JobActivity) act).getJob().equals(job)) {
+                    if (actRemoved) {
+                        // other activities also refer to job --> do not remove job
+                        // thus no need to iterate any further
+                        return true;
                     }
+                    jobIsAlsoAssociateToOtherActs = true;
                 }
             }
         }
-        if (!jobIsAlsoAssociateToOtherActs && actRemoved) {
+        if (actRemoved) {
             jobs.remove(job);
         }
         return actRemoved;

diff --git a/.../java/com/graphhopper/jsprit/core/problem/solution/route/activity/TestTourActivities.java b/.../java/com/graphhopper/jsprit/core/problem/solution/route/activity/TestTourActivities.java
@@ -22,6 +22,7 @@
 import com.graphhopper.jsprit.core.problem.job.Shipment;
 import org.junit.Before;
 import org.junit.Test;
+import org.mockito.Mockito;
 
 import static org.junit.Assert.*;
 
@@ -118,6 +119,19 @@ public void whenRemovingShipment_theirCorrespondingActivitiesShouldBeRemoved() {
         assertEquals(0, tour.getActivities().size());
     }
 
+    @Test
+    public void removingNonJobActivityShouldWork() {
+        TourActivity nonJobAct = Mockito.mock(TourActivity.class);
+
+        tour.addActivity(nonJobAct);
+        assertTrue(tour.getActivities().contains(nonJobAct));
+
+        tour.removeActivity(nonJobAct);
+
+        assertTrue(tour.isEmpty());
+        assertFalse(tour.getActivities().contains(nonJobAct));
+    }
+
     @Test
     public void removingActivityShouldWork() {
         tour.addActivity(act);