Conversation
…ormance, introducing internal operations for span, shift, and expand functionalities.
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR refactors Intervals.NET domain extension methods by centralizing shared range-domain logic into a new internal helper (RangeDomainOperations), then updating fixed/variable extension APIs and tests to use the centralized implementation.
Changes:
- Added
Internal.RangeDomainOperationsto consolidate Span/Shift/Expand/ExpandByRatio logic. - Updated Fixed/Variable
RangeDomainExtensionsto delegate implementations to the internal helper (and added Shift/Expand APIs). - Removed
CommonRangeDomainExtensions(and its tests), expanded test coverage for Shift/Expand, and bumped Domain.Extensions package version.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Intervals.NET.Domain.Extensions.Tests/Variable/RangeDomainExtensionsTests.cs | Adds Shift/Expand tests for variable-step business-day domains. |
| tests/Intervals.NET.Domain.Extensions.Tests/Fixed/RangeDomainExtensionsTests.cs | Adds Shift/Expand tests for fixed-step integer domain. |
| tests/Intervals.NET.Domain.Extensions.Tests/CommonRangeDomainExtensionsTests.cs | Removes tests for deleted CommonRangeDomainExtensions API. |
| src/Intervals.NET.Data/RangeData.cs | Removes an Intervals.NET using (currently breaks compilation due to Range<>/RangeValue<> usage). |
| src/Intervals.NET.Data/Extensions/RangeDataExtensions.cs | Removes an Intervals.NET using (currently breaks compilation due to Range<> usage). |
| src/Intervals.NET.Data/Extensions/EnumerableExtensions.cs | Removes an Intervals.NET using (currently breaks compilation due to Range<> usage). |
| src/Domain/Intervals.NET.Domain.Extensions/Variable/RangeDomainExtensions.cs | Refactors Span/ExpandByRatio and adds Shift/Expand, delegating to internal operations; Span return type changes. |
| src/Domain/Intervals.NET.Domain.Extensions/Intervals.NET.Domain.Extensions.csproj | Bumps package version from 0.0.2 to 0.0.3. |
| src/Domain/Intervals.NET.Domain.Extensions/Internal/RangeDomainOperations.cs | New internal centralized implementation for Span/Shift/Expand/ExpandByRatio. |
| src/Domain/Intervals.NET.Domain.Extensions/Fixed/RangeDomainExtensions.cs | Refactors Span/ExpandByRatio and adds Shift/Expand, delegating to internal operations. |
| src/Domain/Intervals.NET.Domain.Extensions/CommonRangeDomainExtensions.cs | Removes the previously public performance-agnostic extension API. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -1,5 +1,4 @@ | |||
| using Intervals.NET; | |||
| using Intervals.NET.Domain.Abstractions; | |||
| using Intervals.NET.Domain.Abstractions; | |||
There was a problem hiding this comment.
Removing using Intervals.NET; breaks compilation because this file references Range<...> and RangeValue<...> types from the Intervals.NET namespace (e.g., RangeData(Range<TRangeType> range, ...)). Add the using back (or fully-qualify those types).
| using Intervals.NET.Domain.Abstractions; | |
| using Intervals.NET.Domain.Abstractions; | |
| using Intervals.NET; |
| @@ -1,5 +1,4 @@ | |||
| using System.Runtime.CompilerServices; | |||
There was a problem hiding this comment.
Removing using Intervals.NET; breaks compilation because this file uses Range<...> (e.g., in method signatures and examples) which lives in the Intervals.NET namespace. Add the using back (or fully-qualify Intervals.NET.Range<T>).
| using System.Runtime.CompilerServices; | |
| using System.Runtime.CompilerServices; | |
| using Intervals.NET; |
| @@ -1,4 +1,3 @@ | |||
| using Intervals.NET; | |||
| using Intervals.NET.Domain.Abstractions; | |||
There was a problem hiding this comment.
Removing using Intervals.NET; breaks compilation because this file references Range<TRangeType> in ToRangeData(...), which is in the Intervals.NET namespace. Add the using back (or fully-qualify the type).
| using Intervals.NET.Domain.Abstractions; | |
| using Intervals.NET.Domain.Abstractions; | |
| using Intervals.NET; |
| public static RangeValue<long> Span<TRangeValue, TDomain>(this Range<TRangeValue> range, TDomain domain) | ||
| where TRangeValue : IComparable<TRangeValue> | ||
| where TDomain : IVariableStepDomain<TRangeValue> | ||
| { | ||
| // If either boundary is unbounded in the direction that expands the range, span is infinite | ||
| if (range.Start.IsNegativeInfinity || range.End.IsPositiveInfinity) | ||
| { | ||
| return RangeValue<double>.PositiveInfinity; | ||
| } | ||
|
|
||
| var firstStep = CalculateFirstStep(range, domain); | ||
| var lastStep = CalculateLastStep(range, domain); | ||
|
|
||
| switch (firstStep.CompareTo(lastStep)) | ||
| { | ||
| // After domain alignment, boundaries can cross (e.g., open range smaller than one step) | ||
| // Example: (Jan 1 00:00, Jan 1 00:01) with day domain -> firstStep=Jan 2, lastStep=Dec 31 | ||
| case > 0: | ||
| return 0.0; | ||
| case 0: | ||
| return HandleSingleStepCase(range, domain); | ||
| } | ||
|
|
||
| // Note: IRangeDomain.Distance returns long, but for variable-step domains we interpret | ||
| // this as the number of complete steps and add 1.0 to get the span including boundaries | ||
| var distance = (double)domain.Distance(firstStep, lastStep); | ||
| return distance + 1.0; | ||
|
|
||
| // Local functions | ||
| static TRangeValue CalculateFirstStep(Range<TRangeValue> r, TDomain d) | ||
| { | ||
| if (r.IsStartInclusive) | ||
| { | ||
| // Include boundary: use floor to include the step we're on/in | ||
| return d.Floor(r.Start.Value); | ||
| } | ||
|
|
||
| // Exclude boundary: floor to get the boundary, then add 1 to skip it | ||
| var flooredStart = d.Floor(r.Start.Value); | ||
| return d.Add(flooredStart, 1); | ||
| } | ||
|
|
||
| static TRangeValue CalculateLastStep(Range<TRangeValue> r, TDomain d) | ||
| { | ||
| if (r.IsEndInclusive) | ||
| { | ||
| // Include boundary: use floor to include the step we're on/in | ||
| return d.Floor(r.End.Value); | ||
| } | ||
|
|
||
| // Exclude boundary: floor to get the boundary, then subtract 1 to exclude it | ||
| var flooredEnd = d.Floor(r.End.Value); | ||
| return d.Add(flooredEnd, -1); | ||
| } | ||
|
|
||
| static double HandleSingleStepCase(Range<TRangeValue> r, TDomain d) | ||
| { | ||
| // If both floor to the same step, check if either bound is actually ON that step | ||
| var startIsOnBoundary = d.Floor(r.Start.Value).CompareTo(r.Start.Value) == 0; | ||
| var endIsOnBoundary = d.Floor(r.End.Value).CompareTo(r.End.Value) == 0; | ||
|
|
||
| if (r is { IsStartInclusive: true, IsEndInclusive: true } && (startIsOnBoundary || endIsOnBoundary)) | ||
| { | ||
| return 1.0; | ||
| } | ||
|
|
||
| // Otherwise, they're in between domain steps, return 0 | ||
| return 0.0; | ||
| } | ||
| } | ||
| where TDomain : IVariableStepDomain<TRangeValue> => Internal.RangeDomainOperations.CalculateSpan(range, domain); | ||
|
|
There was a problem hiding this comment.
Span changed from RangeValue<double> to RangeValue<long> for variable-step domains. This is a breaking public API change and currently makes existing call sites/tests that compare against doubles (e.g., Assert.Equal(5.0, span.Value)) fail to compile. If the change is intentional, update downstream usages/tests and consider calling it out prominently in release notes; otherwise, keep the return type consistent with prior versions.
src/Domain/Intervals.NET.Domain.Extensions/Internal/RangeDomainOperations.cs
Show resolved
Hide resolved
blaze6950
deleted the
refactor/refactor-slightly-in-the-scope-of-real-usage
branch
2 participants
This pull request refactors and centralizes range domain operations for the Intervals.NET library, improving code maintainability and ensuring consistent performance semantics. The main change is the introduction of a new internal helper class,
RangeDomainOperations, which consolidates shared logic for span calculation, shifting, and expansion of ranges. Public extension methods in both common and fixed-step domain extension classes are updated to delegate their implementation to this internal class, reducing code duplication and clarifying performance guarantees.Centralization of range domain operations:
RangeDomainOperationsinsrc/Domain/Intervals.NET.Domain.Extensions/Internal/RangeDomainOperations.cs, which implements shared logic forCalculateSpan,Shift,Expand, andExpandByRatiomethods for both fixed and variable-step domains. This ensures consistent behavior and performance semantics across domain types.Refactoring of public extension methods:
Span,ExpandByRatio,Shift, andExpandmethods insrc/Domain/Intervals.NET.Domain.Extensions/Fixed/RangeDomainExtensions.csto delegate their implementation to the corresponding methods inRangeDomainOperations, simplifying the code and reducing duplication. [1] [2]CommonRangeDomainExtensionsclass fromsrc/Domain/Intervals.NET.Domain.Extensions/CommonRangeDomainExtensions.cs, as its logic is fully covered by the new internal helper class.Documentation improvements:
ExpandByRatioinRangeDomainExtensions, clarifying the meaning and usage of ratio coefficients and providing examples for negative ratios. [1] [2]These changes improve maintainability, clarify performance characteristics, and make the codebase easier to extend and reason about.