Split ByDayFilter, implements #52

The original ByDayFilter was way to complex and hard to maintain. This commit breaks it into two much simpler classes. A part of the heavy lifting is now done in RecurrenceRule and should be subject to another refactoring.
A nice side effect is a small performance improvement when filtering ByDay.

src/main/java/org/dmfs/rfc5545/recur/ByDayFilter.java

@@ -1,5 +1,6 @@
 /*
- * Copyright (C) 2013 Marten Gajda <[email protected]>
+ * Copyright 2020 Marten Gajda <[email protected]>
+ *
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -12,18 +13,15 @@
  * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
  * See the License for the specific language governing permissions and
  * limitations under the License.
- * 
  */
 
 package org.dmfs.rfc5545.recur;
 
 import org.dmfs.rfc5545.Instance;
+import org.dmfs.rfc5545.Weekday;
 import org.dmfs.rfc5545.calendarmetrics.CalendarMetrics;
-import org.dmfs.rfc5545.recur.ByExpander.Scope;
-import org.dmfs.rfc5545.recur.RecurrenceRule.Part;
-import org.dmfs.rfc5545.recur.RecurrenceRule.WeekdayNum;
 
-import java.util.List;
+import java.util.Set;
 
 
 /**
@@ -33,151 +31,30 @@
  */
 final class ByDayFilter implements ByFilter
 {
-    /**
-     * The scope of this rule.
-     */
-    private final Scope mScope;
-
-    /**
-     * A flag indicating that at least one of the week days has a positional prefix.
-     */
-    private final boolean mHasPositions;
-
-    /**
-     * An array of the weekdays and their positions in a packed form that allows us to find them with a simple integer comparison
-     */
-    private final int[] mPackedDays;
-
-
-    /**
-     * Get a packed representation of a {@link WeekdayNum}.
-     *
-     * @param pos
-     *         The position of the day or <code>0</code>.
-     * @param day
-     *         The number of the weekday.
-     *
-     * @return An int that contains the position and the weekday.
-     */
-    private static int packWeekday(int pos, int day)
-    {
-        return (pos << 8) + day;
-    }
-
-
-    /**
-     * Get the weekday part of a packed day.
-     *
-     * @param packedDay
-     *         The packed day int.
-     *
-     * @return The weekday.
-     */
-    @SuppressWarnings("unused")
-    private static int unpackWeekday(int packedDay)
-    {
-        return packedDay & 0xff;
-    }
-
 
     /**
-     * Get the positional part of a packed day.
-     *
-     * @param packedDay
-     *         The packed day int.
-     *
-     * @return The position.
+     * The {@link CalendarMetrics} to use.
      */
-    @SuppressWarnings("unused")
-    private static int unpackPos(int packedDay)
-    {
-        return packedDay >>> 8;
-    }
+    private final CalendarMetrics mCalendarMetrics;
 
+    private final Set<Weekday> mWeekdays;
 
-    /**
-     * The {@link CalendarMetrics} to use.
-     */
-    final CalendarMetrics mCalendarMetrics;
+    private final Weekday[] mWeekdayArray = Weekday.values();
 
 
-    public ByDayFilter(RecurrenceRule rule, CalendarMetrics calendarMetrics)
+    public ByDayFilter(CalendarMetrics calendarMetrics, Set<Weekday> weekdays)
     {
         mCalendarMetrics = calendarMetrics;
-        List<WeekdayNum> byDay = rule.getByDayPart();
-
-        boolean hasByMonth = rule.hasPart(Part.BYMONTH);
-        Freq freq = rule.getFreq();
-
-        mScope = rule.hasPart(
-                Part.BYWEEKNO) || freq == Freq.WEEKLY ? (hasByMonth || freq == Freq.MONTHLY ? Scope.WEEKLY_AND_MONTHLY : Scope.WEEKLY)
-                : (hasByMonth || freq == Freq.MONTHLY ? Scope.MONTHLY : Scope.YEARLY);
-
-        boolean hasPositions = false;
-
-        mPackedDays = new int[byDay.size()];
-        int count = 0;
-        for (WeekdayNum w : byDay)
-        {
-            if (w.pos != 0)
-            {
-                hasPositions = true;
-            }
-            mPackedDays[count] = packWeekday(w.pos, w.weekday.ordinal());
-            ++count;
-        }
-        mHasPositions = hasPositions;
+        mWeekdays = weekdays;
     }
 
 
     @Override
     public boolean filter(long instance)
     {
-        // this is called if FREQ is <= DAILY or any of BYMONTHDAY or BYYEARDAY is present, so we don't have to filter by month here
-        int year = Instance.year(instance);
-        int month = Instance.month(instance);
-        int dayOfMonth = Instance.dayOfMonth(instance);
-        CalendarMetrics calendarMetrics = mCalendarMetrics;
-        int dayOfWeek = calendarMetrics.getDayOfWeek(year, month, dayOfMonth);
-        int[] packedDays = mPackedDays;
-
-        if (!mHasPositions)
-        {
-            return StaticUtils.linearSearch(packedDays, packWeekday(0, dayOfWeek)) < 0;
-        }
-        else
-        {
-            switch (mScope)
-            {
-                case WEEKLY:
-                    /*
-                     * Note: if we're in a weekly scope we shouldn't be here. So we just ignore any days with positions.
-					 */
-                    return StaticUtils.linearSearch(packedDays, packWeekday(0, dayOfWeek)) < 0;
-
-                case WEEKLY_AND_MONTHLY:
-                case MONTHLY:
-                {
-                    int nthDay = (dayOfMonth - 1) / 7 + 1;
-                    int lastNthDay = (dayOfMonth - calendarMetrics.getDaysPerPackedMonth(year, month)) / 7 - 1;
-                    return (nthDay <= 0 || StaticUtils.linearSearch(packedDays, packWeekday(nthDay, dayOfWeek)) < 0)
-                            && (lastNthDay >= 0 || StaticUtils.linearSearch(packedDays,
-                            packWeekday(lastNthDay, dayOfWeek)) < 0)
-                            && StaticUtils.linearSearch(packedDays, packWeekday(0, dayOfWeek)) < 0;
-                }
-                case YEARLY:
-                {
-                    int yearDay = calendarMetrics.getDayOfYear(year, month, dayOfMonth);
-                    int nthDay = (yearDay - 1) / 7 + 1;
-                    int lastNthDay = (yearDay - calendarMetrics.getDaysPerYear(year)) / 7 - 1;
-                    return (nthDay <= 0 || StaticUtils.linearSearch(packedDays, packWeekday(nthDay, dayOfWeek)) < 0)
-                            && (lastNthDay >= 0 || StaticUtils.linearSearch(packedDays,
-                            packWeekday(lastNthDay, dayOfWeek)) < 0)
-                            && StaticUtils.linearSearch(packedDays, packWeekday(0, dayOfWeek)) < 0;
-                }
-                default:
-                    return false;
-            }
-        }
+        return !mWeekdays.contains(mWeekdayArray[mCalendarMetrics.getDayOfWeek(
+                Instance.year(instance),
+                Instance.month(instance),
+                Instance.dayOfMonth(instance))]);
     }
 }

src/main/java/org/dmfs/rfc5545/recur/ByDayPrefixedFilter.java

@@ -0,0 +1,93 @@
+/*
+ * Copyright 2020 Marten Gajda <[email protected]>
+ *
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.dmfs.rfc5545.recur;
+
+import org.dmfs.jems.function.BiFunction;
+import org.dmfs.rfc5545.Instance;
+import org.dmfs.rfc5545.Weekday;
+import org.dmfs.rfc5545.calendarmetrics.CalendarMetrics;
+
+import java.util.Map;
+import java.util.Set;
+
+
+/**
+ * A filter that limits recurrence rules by day of week.
+ *
+ * @author Marten Gajda
+ */
+final class ByDayPrefixedFilter implements ByFilter
+{
+    public enum Scope
+    {
+        MONTH(
+                (instance, metrics) ->
+                        (Instance.dayOfMonth(instance) - 1) / 7 + 1,
+                (instance, metrics) ->
+                        (Instance.dayOfMonth(instance) - metrics.getDaysPerPackedMonth(Instance.year(instance), Instance.month(instance))) / 7 - 1),
+        YEAR(
+                (instance, metrics) ->
+                        (metrics.getDayOfYear(Instance.year(instance), Instance.month(instance), Instance.dayOfMonth(instance)) - 1) / 7 + 1,
+                (instance, metrics) ->
+                        (metrics.getDayOfYear(Instance.year(instance), Instance.month(instance), Instance.dayOfMonth(instance))
+                                - metrics.getDaysPerYear(Instance.year(instance))) / 7 - 1);
+
+        private final BiFunction<Long, CalendarMetrics, Integer> mNthDay;
+        private final BiFunction<Long, CalendarMetrics, Integer> mNthLastDay;
+
+
+        Scope(BiFunction<Long, CalendarMetrics, Integer> nthDay, BiFunction<Long, CalendarMetrics, Integer> nthLastDay)
+        {
+            mNthDay = nthDay;
+            mNthLastDay = nthLastDay;
+        }
+    }
+
+
+    /**
+     * The {@link CalendarMetrics} to use.
+     */
+    private final CalendarMetrics mCalendarMetrics;
+    private final Map<Weekday, Set<Integer>> mPrefixedWeekDays;
+    private final Scope mScope;
+    private final Weekday[] mWeekdayArray = Weekday.values();
+
+
+    public ByDayPrefixedFilter(
+            CalendarMetrics calendarMetrics,
+            Map<Weekday, Set<Integer>> prefixedWeekDays,
+            Scope scope)
+    {
+        mCalendarMetrics = calendarMetrics;
+        mPrefixedWeekDays = prefixedWeekDays;
+        mScope = scope;
+    }
+
+
+    @Override
+    public boolean filter(long instance)
+    {
+        Set<Integer> prefixes = mPrefixedWeekDays.get(mWeekdayArray[mCalendarMetrics.getDayOfWeek(
+                Instance.year(instance),
+                Instance.month(instance),
+                Instance.dayOfMonth(instance))]);
+        return prefixes == null
+                || (!prefixes.contains(mScope.mNthDay.value(instance, mCalendarMetrics))
+                && !prefixes.contains(mScope.mNthLastDay.value(instance, mCalendarMetrics)));
+    }
+}

src/main/java/org/dmfs/rfc5545/recur/RecurrenceRule.java

@@ -29,6 +29,7 @@
 import java.util.EnumMap;
 import java.util.EnumSet;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Locale;
 import java.util.Map;
@@ -478,7 +479,50 @@ RuleIterator getExpander(RecurrenceRule rule, RuleIterator previous, CalendarMet
                     @Override
                     ByFilter getFilter(RecurrenceRule rule, CalendarMetrics calendarMetrics) throws UnsupportedOperationException
                     {
-                        return new ByDayFilter(rule, calendarMetrics);
+                        Freq freq = rule.getFreq();
+                        // TODO: this method looks ugly and can use some improvement
+                        // TODO: consider doing this separation at parsing time
+                        Set<Weekday> nonPrefixed = EnumSet.noneOf(Weekday.class);
+                        Map<Weekday, Set<Integer>> prefixed = new EnumMap<>(Weekday.class);
+
+                        for (WeekdayNum wn : rule.getByDayPart())
+                        {
+                            if (wn.pos == 0)
+                            {
+                                nonPrefixed.add(wn.weekday);
+                            }
+                            else
+                            {
+                                Set<Integer> prefixes = prefixed.get(wn.weekday);
+                                if (prefixes == null) // TODO use java 8 API, check Android support first
+                                {
+                                    prefixes = new HashSet<>();
+                                    prefixed.put(wn.weekday, prefixes);
+                                }
+                                prefixes.add(wn.pos);
+                            }
+                        }
+
+                        if (prefixed.isEmpty()
+                                || freq != Freq.YEARLY && freq != Freq.MONTHLY
+                                || freq == Freq.YEARLY && rule.hasPart(BYWEEKNO))
+                        {
+                            return new ByDayFilter(calendarMetrics, nonPrefixed);
+                        }
+                        ByFilter baseFilter = new ByDayPrefixedFilter(
+                                calendarMetrics,
+                                prefixed,
+                                freq == Freq.YEARLY && rule.getByPart(BYMONTH) == null ?
+                                        ByDayPrefixedFilter.Scope.YEAR :
+                                        ByDayPrefixedFilter.Scope.MONTH);
+
+                        if (nonPrefixed.isEmpty())
+                        {
+                            return baseFilter;
+                        }
+
+                        ByFilter nonPrefixFilter = new ByDayFilter(calendarMetrics, nonPrefixed);
+                        return instance -> nonPrefixFilter.filter(instance) && baseFilter.filter(instance);
                     }
 
 
@@ -612,7 +656,7 @@ RuleIterator getExpander(RecurrenceRule rule, RuleIterator previous, CalendarMet
                     @Override
                     ByFilter getFilter(RecurrenceRule rule, CalendarMetrics calendarMetrics) throws UnsupportedOperationException
                     {
-                        return new ByDayFilter(rule, calendarMetrics);
+                        return BYDAY.getFilter(rule, calendarMetrics);
                     }
 
 

src/test/java/org/dmfs/rfc5545/recur/RecurrenceRuleTest.java

@@ -25,6 +25,7 @@
 import static org.dmfs.rfc5545.Weekday.MO;
 import static org.dmfs.rfc5545.Weekday.TH;
 import static org.dmfs.rfc5545.Weekday.TU;
+import static org.dmfs.rfc5545.Weekday.WE;
 import static org.dmfs.rfc5545.hamcrest.RecurrenceRuleMatcher.are;
 import static org.dmfs.rfc5545.hamcrest.RecurrenceRuleMatcher.instances;
 import static org.dmfs.rfc5545.hamcrest.RecurrenceRuleMatcher.results;
@@ -82,5 +83,11 @@ public void test() throws InvalidRecurrenceRuleException
                 },
                 hasValue(hasToString("FREQ=MONTHLY;RSCALE=GREGORIAN;SKIP=FORWARD;COUNT=5"))
         );
+
+        assertThat(new RecurrenceRule("FREQ=MONTHLY;BYDAY=1MO,-1MO,WE"),
+                is(validRule(DateTime.parse("20200902"),
+                        walking(),
+                        instances(are(onWeekDay(MO, WE))),
+                        startingWith("20200902", "20200907", "20200909", "20200916", "20200923", "20200928", "20200930", "20201005", "20201007"))));
     }
 }