Skip to content

chore: Eliminate nested if statements in GetAbsWeekDaysMonthly/Weekly#896

Merged
axunonb merged 1 commit intomainfrom
wip/axunonb/pr/resolve-nested-if
Dec 23, 2025
Merged

chore: Eliminate nested if statements in GetAbsWeekDaysMonthly/Weekly#896
axunonb merged 1 commit intomainfrom
wip/axunonb/pr/resolve-nested-if

Conversation

@axunonb
Copy link
Collaborator

@axunonb axunonb commented Dec 23, 2025

Extract week number and month match conditions into named boolean variables, then combine with single if statement.

No functional changes.

Extract week number and month match conditions into named boolean variables, then combine with single if statement.

No functional changes.
@axunonb axunonb requested a review from Copilot December 23, 2025 23:01
@sonarqubecloud
Copy link

@codecov
Copy link

codecov bot commented Dec 23, 2025

Codecov Report

❌ Patch coverage is 75.00000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 75.0% 0 Missing and 2 partials ⚠️

Impacted file tree graph

@@          Coverage Diff          @@
##            main    #896   +/-   ##
=====================================
  Coverage   68.5%   68.5%           
=====================================
  Files        115     115           
  Lines       4394    4396    +2     
  Branches    1011    1011           
=====================================
+ Hits        3008    3010    +2     
  Misses      1028    1028           
  Partials     358     358           
Files with missing lines Coverage Δ
Ical.Net/Evaluation/RecurrencePatternEvaluator.cs 92.3% <75.0%> (+<0.1%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors two methods (GetAbsWeekDaysMonthly and GetAbsWeekDaysWeekly) to eliminate nested if statements by extracting conditional expressions into named boolean variables. While this improves code readability, it introduces a performance consideration by removing the short-circuit evaluation behavior of the original nested structure.

  • Extracted week number and month match conditions into matchesWeekNo and matchesMonth variables
  • Replaced nested if statements with a single combined conditional check

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +732 to +735
var matchesWeekNo = byWeekNoNormalized == null || byWeekNoNormalized.Contains(Calendar.GetIso8601WeekOfYear(date, pattern.FirstDayOfWeek));
var matchesMonth = pattern.ByMonth.Count == 0 || pattern.ByMonth.Contains(date.Month);

if (matchesWeekNo && matchesMonth)
Copy link

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refactoring eliminates the short-circuit behavior of the original nested if statements. Both conditions are now evaluated unconditionally on every iteration, even when the first condition fails. This means Calendar.GetIso8601WeekOfYear is called even when byWeekNoNormalized is null, and pattern.ByMonth.Contains is checked even when matchesWeekNo is false. Consider keeping the nested structure or using a single combined if condition without intermediate variables to preserve the original short-circuit evaluation and avoid unnecessary computations in the loop.

Suggested change
var matchesWeekNo = byWeekNoNormalized == null || byWeekNoNormalized.Contains(Calendar.GetIso8601WeekOfYear(date, pattern.FirstDayOfWeek));
var matchesMonth = pattern.ByMonth.Count == 0 || pattern.ByMonth.Contains(date.Month);
if (matchesWeekNo && matchesMonth)
if ((byWeekNoNormalized == null || byWeekNoNormalized.Contains(Calendar.GetIso8601WeekOfYear(date, pattern.FirstDayOfWeek)))
&& (pattern.ByMonth.Count == 0 || pattern.ByMonth.Contains(date.Month)))

Copilot uses AI. Check for mistakes.
@axunonb axunonb merged commit 98e1f97 into main Dec 23, 2025
17 of 18 checks passed
@axunonb axunonb deleted the wip/axunonb/pr/resolve-nested-if branch December 23, 2025 23:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant