Solution for new `GetPreviousOccurence`

[DaveRMaltby]

…methods. https://github.com/atifaziz/NCrontab/issues/64 Moves from the start DateTime in a reverse fashion to avoid a need to move forward and guess how far in the past to begin the search from.

[DaveRMaltby]

Build appears to be failing due to an old .NET Standard library reference.

/home/appveyor/projects/ncrontab/NCrontab/NCrontab.csproj : error NU3037: Package 'NETStandard.Library 2.0.3' from source 'https://api.nuget.org/v3/index.json': The repository primary signature validity period has expired.

[SammyROCK]

I needed this so I could use CRON to define working windows with a limit of operations between each window I had to get when the CRON window started and when it ended to load the operations that were executed or scheduled in the window. By computing when was the last time the CRON was valid (when window started) without GetPrevOccurence, I had to loop back in time in a bizarre algorithm which had a huge CPU impact. By using this code the performance was similar to GetNextOccurence Thx @DaveRMaltby

[DaveRMaltby]

@atifaziz, I understand your position and made this contribution because I want my company (Servant Software LLC) to go beyond just mere consumption of these open source projects, but to be an active contributor. When contributing this PR, we had some competing priorities and so you're right, for this work to get merged into master it needs more efforts. I'll personally make sure that these are dealt with and provide you with updates.

Honestly, I thought that this repo had gone dormant and wanted to get this PR out there before forgetting to post it, so that others (like @SammyROCK) could at least get a fork of it, if they had such a specialized need, like myself. Very glad to see that you breathed new life into the repo today. Thank you! We plan to be here to help you as needed and totally understand if this feature doesn't make the cut.

[atifaziz]

@atifaziz, I understand your position and made this contribution because I want my company (Servant Software LLC) to go beyond just mere consumption of these open source projects, but to be an active contributor.

@DaveRMaltby You can imagine this is like music to any maintainer's ears and I really commend your attitude and will.

When contributing this PR, we had some competing priorities and so you're right, for this work to get merged into master it needs more efforts.

Understandable.

Honestly, I thought that this repo had gone dormant

I time-share between many open source project (too many for my own good). There are periods when some get more love than others as I try to avoid moving in and out of different problem spaces so I can do each proper justice. On top of that, this particular library was always meant to do just one thing, so I consider it to be really feature complete (including stability and compatibility) and that too can lead to the impression of it being abandoned/dormant. The one (non-urgent) feature I've been meaning to add since a while is enabling unions of multiple schedules. This may be something to think about with respect to computing previous occurrences.

I'll personally make sure that these are dealt with and provide you with updates.

Looking forward.

[atifaziz]

For reference, below is the difference between TryGetNextOccurrence and TryGetPrevOccurrence:

--- .\TryGetNextOccurrence.cs
+++ .\TryGetPreviousOccurrence.cs
@@ -1,4 +1,4 @@
-        DateTime? TryGetNextOccurrence(DateTime baseTime, DateTime endTime)
+        DateTime? TryGetPrevOccurrence(DateTime baseTime, DateTime startTime)
         {
             const int nil = -1;
 
@@ -9,109 +9,118 @@
             var baseMinute = baseTime.Minute;
             var baseSecond = baseTime.Second;
 
-            var endYear = endTime.Year;
-            var endMonth = endTime.Month;
-            var endDay = endTime.Day;
+            var startYear = startTime.Year;
+            var startMonth = startTime.Month;
+            var startDay = startTime.Day;
 
             var year = baseYear;
             var month = baseMonth;
             var day = baseDay;
             var hour = baseHour;
             var minute = baseMinute;
-            var second = baseSecond + 1;
+            var second = baseSecond - 1;
 
             //
             // Second
             //
 
             var seconds = _seconds ?? SecondZero;
-            second = seconds.Next(second);
+            second = seconds.Prev(second);
 
             if (second == nil)
             {
-                second = seconds.GetFirst();
-                minute++;
+                second = seconds.GetLast();
+                minute--;
             }
 
             //
             // Minute
             //
 
-            minute = _minutes.Next(minute);
+            minute = _minutes.Prev(minute);
 
             if (minute == nil)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
-                hour++;
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
+                hour--;
             }
-            else if (minute > baseMinute)
+            else if (minute < baseMinute)
             {
-                second = seconds.GetFirst();
+                second = seconds.GetLast();
             }
 
             //
             // Hour
             //
 
-            hour = _hours.Next(hour);
+            hour = _hours.Prev(hour);
 
+            // this variable for trick
+            // to keep day when it adjusted day 31 for
+            // "short" months
+            bool adjusting = false;
+
+            RetryDayMonth:
+
             if (hour == nil)
             {
-                minute = _minutes.GetFirst();
-                hour = _hours.GetFirst();
-                day++;
+                minute = _minutes.GetLast();
+                hour = _hours.GetLast();
+                day--;
             }
             else if (hour > baseHour)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
             }
 
             //
             // Day
             //
 
-            day = _days.Next(day);
+            
 
-            RetryDayMonth:
+            day = _days.Prev(day);
 
+            
             if (day == nil)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
-                hour = _hours.GetFirst();
-                day = _days.GetFirst();
-                month++;
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
+                hour = _hours.GetLast();
+                day = _days.GetLast();
+                month--;
             }
-            else if (day > baseDay)
+            else if (day < baseDay)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
-                hour = _hours.GetFirst();
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
+                hour = _hours.GetLast();
             }
 
             //
             // Month
             //
 
-            month = _months.Next(month);
+            month = _months.Prev(month);
 
             if (month == nil)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
-                hour = _hours.GetFirst();
-                day = _days.GetFirst();
-                month = _months.GetFirst();
-                year++;
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
+                hour = _hours.GetLast();
+                day = _days.GetLast();
+                month = _months.GetLast();
+                year--;
             }
-            else if (month > baseMonth)
+            else if (month < baseMonth)
             {
-                second = seconds.GetFirst();
-                minute = _minutes.GetFirst();
-                hour = _hours.GetFirst();
-                day = _days.GetFirst();
+                second = seconds.GetLast();
+                minute = _minutes.GetLast();
+                hour = _hours.GetLast();
+                if (!adjusting)
+                    day = _days.GetLast();
             }
 
             //
@@ -119,7 +128,7 @@
             // object. Otherwise we would get an exception.
             //
 
-            if (year > Calendar.MaxSupportedDateTime.Year)
+            if (year < Calendar.MinSupportedDateTime.Year)
                 return null;
 
             //
@@ -143,25 +152,26 @@
 
             if (day > 28 && dateChanged && day > Calendar.GetDaysInMonth(year, month))
             {
-                if (year >= endYear && month >= endMonth && day >= endDay)
-                    return endTime;
+                if (year <= startYear && month <= startMonth && day <= startDay)
+                    return startTime;
 
-                day = nil;
+                hour = nil;
+                adjusting = true;
                 goto RetryDayMonth;
             }
 
-            var nextTime = new DateTime(year, month, day, hour, minute, second, 0, baseTime.Kind);
+            var prevTime = new DateTime(year, month, day, hour, minute, second, 0, baseTime.Kind);
 
-            if (nextTime >= endTime)
-                return endTime;
+            if (prevTime <= startTime)
+                return startTime;
 
             //
             // Day of week
             //
 
-            if (_daysOfWeek.Contains((int)nextTime.DayOfWeek))
-                return nextTime;
+            if (_daysOfWeek.Contains((int)prevTime.DayOfWeek))
+                return prevTime;
 
-            return TryGetNextOccurrence(new DateTime(year, month, day, 23, 59, 59, 0, baseTime.Kind), endTime);
+            return TryGetPrevOccurrence(new DateTime(year, month, day, 0, 0, 0, 0, baseTime.Kind), startTime);
         }

Unfortunately, this at the line level, but actually the delta is even less when you look at the level of characters and then the duplication really stands out. If each method is extracted into a separate files, say named TryGetNextOccurrence.cs and TryGetPreviousOccurrence.cs, then you can visualise the character-level differences with (without the files needing to be part of the Git repo):

git diff --no-index --word-diff=color --word-diff-regex=. TryGetNextOccurrence.cs TryGetPreviousOccurrence.cs

[DaveRMaltby]

Thanks for this contribution, but I'm afraid it still needs a lot of work before it can be merged. It adds considerable new and fundamental logic that's duplicated between TryGetNextOccurrence and TryGetPrevOccurrence, but with very few tests ensuring full coverage (e.g. TryGetNextOccurrence has 0 uncovered blocks but TryGetPrevOccurrence has 11). Not only it adds to maintenance, I'm not confident that it's bug-free. Since you may not always be around to help maintain and fix any reported bugs, I want to avoid that burden falling entirely on me. If you have the will and motivation to address the following points then I'd be happy to work with you to get this into a mergeable state:

1. Consolidating as much of the code duplication as possible.
2. Ensuring full test coverage.
3. Exercising any edge cases you can think of.

PS Also, please bear in mind that issue #64 was never committed as a feature so there is a slight chance that it may not still make the cut. That said, I am open to seeing if this can be done with some reasonable effort.

@atifaziz, are you using a particular tool to determine the uncovered blocks? I would like use the same tool locally to avoid guessing at when I've covered all blocks with unit tests. Thanks!

[DaveRMaltby]

I've completed #1, but haven't pushed it yet, as I'd like to complete #2 & #3 before presenting it here.

[atifaziz]

are you using a particular tool to determine the uncovered blocks? I would like use the same tool locally to avoid guessing at when I've covered all blocks with unit tests.

I used the one built into Visual Studio for a very quick and cursory analysis, but unfortunately, the feature is only available in the Enterprise Edition. I'll refresh the coverage & reporting as part of the CI/CD for this repo so it's more accessible and drop a note here when it's available.

[atifaziz]

I'll refresh the coverage & reporting as part of the CI/CD for this repo so it's more accessible and drop a note here when it's available.

This is now done with #115 and available in master with c6b44363058ddf6b40bc6d27101eb70698f6c93d. You may want to merge here. The coverage report can also be browsed online.

[codecov[bot]]

Codecov Report

Patch coverage: 99.41% and project coverage change: +1.42 :tada:

Comparison is base (c6b4436) 88.37% compared to head (afbbec5) 89.80%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #108      +/-   ##
==========================================
+ Coverage   88.37%   89.80%   +1.42%     
==========================================
  Files           5        8       +3     
  Lines         430      500      +70     
==========================================
+ Hits          380      449      +69     
- Misses         50       51       +1     

Impacted Files	Coverage Δ
NCrontab/CrontabField.cs	66.00% <80.00%> (-3.73%)	:arrow_down:
NCrontab/CrontabField.Iterator.cs	100.00% <100.00%> (ø)
NCrontab/CrontabFieldImpl.cs	89.72% <100.00%> (ø)
NCrontab/CrontabSchedule.cs	99.50% <100.00%> (+0.09%)	:arrow_up:
NCrontab/Utils/DateTimeComponents.cs	100.00% <100.00%> (ø)
NCrontab/Utils/Incrementor.cs	100.00% <100.00%> (ø)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[DaveRMaltby]

@atifaziz, I believe all your requests have been completed. I have abstracted the logic such that there is now only one method used by Next and Prev for getting an occurrence called CrontabSchedule.TryGetOccurrence. Please let me know if there is something else needed here. Thanks for your consideration.

Note: @SammyROCK, there were some bugs that was in the version of my fork that you cloned. You may want to pull down the latest.

[zorgoz]

Hi there! What is this "prev" feature's status? I also would need it, and I see that it is almost ready.

[ryanzwe]

Hi just coming in to chime this would be an awesome feature, I wrote a hacky solution to achieve the same thing, but would love a built in version!