FileTarget - Reduce code complexity for ArchiveNumbering property by snakefoot · Pull Request #6064 · NLog/NLog

snakefoot · 2025-12-17T07:21:07Z

No description provided.

coderabbitai · 2025-12-17T07:21:20Z

Walkthrough

The ArchiveNumbering setter in src/NLog/Targets/FileTarget.cs was refactored to trim the input into a local variable, return early when unchanged or empty, and use the trimmed value when deciding ArchiveSuffixFormat.

Changes

Cohort / File(s)	Summary
FileTarget archive numbering optimization `src/NLog/Targets/FileTarget.cs`	Refactored `ArchiveNumbering` setter: trim input once into a local variable, early-return when value is null/empty or unchanged (case-insensitive), and use the trimmed local value for `ArchiveSuffixFormat` selection between legacy date and sequence formats.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Single-file, focused change in one property setter.
Verify case-insensitive comparison, early-return behavior, and that ArchiveSuffixFormat selection uses the trimmed local value.

Poem

🐰 I nibble trims with patient care,
One tidy hop, no extra spare,
I stop when nothing new to do,
Use the clean string, fresh as dew,
Code hops onward — neat and fair!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	No pull request description was provided by the author, making it impossible to assess relevance to the changeset.	Add a description explaining the rationale for the ArchiveNumbering refactoring and how it reduces complexity.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: refactoring the ArchiveNumbering setter in FileTarget to reduce code complexity.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1032d00 and 02b891f.

📒 Files selected for processing (1)

src/NLog/Targets/FileTarget.cs (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

src/NLog/Targets/FileTarget.cs

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

GitHub Check: task-list-completed

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

Actionable comments posted: 0

🧹 Nitpick comments (1)

src/NLog/Targets/FileTarget.cs (1)
393-402: LGTM! Refactoring successfully reduces complexity.

The changes effectively simplify the setter by trimming once, providing early exit for unchanged values, and consistently using the local archiveNumbering variable. The logic is correct and the control flow ensures line 400 is safe from null reference exceptions.

Minor: The null check on line 394 is redundant since string.IsNullOrEmpty already handles null cases. Consider simplifying:
-if (archiveNumbering is null || string.IsNullOrEmpty(archiveNumbering) || string.Equals(_archiveNumbering, archiveNumbering, StringComparison.OrdinalIgnoreCase))
+if (string.IsNullOrEmpty(archiveNumbering) || string.Equals(_archiveNumbering, archiveNumbering, StringComparison.OrdinalIgnoreCase))

📜 Review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 624863f and 1032d00.

📒 Files selected for processing (1)

src/NLog/Targets/FileTarget.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (1)

src/NLog/Targets/FileTarget.cs (2)

src/NLog/Internal/PropertiesDictionary.cs (4)

Equals (748-748)

Equals (750-758)

Equals (772-775)

Equals (786-800)

src/NLog/Filters/WhenRepeatedFilter.cs (2)

Equals (391-407)

Equals (409-412)

sonarqubecloud · 2025-12-17T18:12:31Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

lechu445 · 2025-12-19T00:16:24Z

src/NLog/Targets/FileTarget.cs

-                _archiveNumbering = string.IsNullOrEmpty(value) ? null : value.Trim();
-                if (_archiveNumbering is null || string.IsNullOrEmpty(_archiveNumbering))
+                var archiveNumbering = value?.Trim() ?? string.Empty;
+                if (string.IsNullOrEmpty(archiveNumbering) || string.Equals(_archiveNumbering, archiveNumbering, StringComparison.OrdinalIgnoreCase))


Suggested change

if (string.IsNullOrEmpty(archiveNumbering) || string.Equals(_archiveNumbering, archiveNumbering, StringComparison.OrdinalIgnoreCase))

if (string.IsNullOrEmpty(archiveNumbering) || archiveNumbering.Equals(_archiveNumbering, StringComparison.OrdinalIgnoreCase))

this will reduce one null check that is called in the static string.Equals method

Old habit of using string.Equals not that worried about a bonus null-check for an obsolete property. But thank you for noticing.

lechu445 · 2025-12-19T00:19:25Z

src/NLog/Targets/FileTarget.cs

                if (_archiveSuffixFormat is null || ReferenceEquals(_archiveSuffixFormat, _legacyDateArchiveSuffixFormat) || ReferenceEquals(_archiveSuffixFormat, _legacySequenceArchiveSuffixFormat))
                {
-                    ArchiveSuffixFormat = _archiveNumbering.IndexOf("date", StringComparison.OrdinalIgnoreCase) >= 0 ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;
+                    ArchiveSuffixFormat = archiveNumbering.IndexOf("date", StringComparison.OrdinalIgnoreCase) >= 0 ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;


Suggested change

ArchiveSuffixFormat = archiveNumbering.IndexOf("date", StringComparison.OrdinalIgnoreCase) >= 0 ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;

ArchiveSuffixFormat = archiveNumbering.Contains("date", StringComparison.OrdinalIgnoreCase) ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;

this can be simplified

Recall that Contains with StringComparison is not available on NET35, which is why using IndexOf. But thank you for noticing.

snakefoot added file-target refactoring file-archiving Issues with archiving with the file target labels Dec 17, 2025

pull-request-size bot added the size/S label Dec 17, 2025

coderabbitai bot reviewed Dec 17, 2025

View reviewed changes

FileTarget - Reduce ArchiveNumbering code complexity

02b891f

snakefoot force-pushed the FileTargetArcehiveNumbering branch from 1032d00 to 02b891f Compare December 17, 2025 07:31

snakefoot merged commit 223695f into NLog:dev Dec 17, 2025
5 of 6 checks passed

lechu445 reviewed Dec 19, 2025

View reviewed changes

snakefoot added this to the 6.1.0 milestone Jan 31, 2026

snakefoot changed the title ~~FileTarget - Reduce ArchiveNumbering code complexity~~ FileTarget - Reduce code complexity for ArchiveNumbering property Jan 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FileTarget - Reduce code complexity for ArchiveNumbering property#6064

FileTarget - Reduce code complexity for ArchiveNumbering property#6064
snakefoot merged 1 commit intoNLog:devfrom
snakefoot:FileTargetArcehiveNumbering

snakefoot commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

sonarqubecloud bot commented Dec 17, 2025

Uh oh!

Uh oh!

lechu445 Dec 19, 2025

Uh oh!

snakefoot Dec 19, 2025

Uh oh!

lechu445 Dec 19, 2025

Uh oh!

snakefoot Dec 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	if (string.IsNullOrEmpty(archiveNumbering) \|\| string.Equals(_archiveNumbering, archiveNumbering, StringComparison.OrdinalIgnoreCase))
	if (string.IsNullOrEmpty(archiveNumbering) \|\| archiveNumbering.Equals(_archiveNumbering, StringComparison.OrdinalIgnoreCase))

	ArchiveSuffixFormat = archiveNumbering.IndexOf("date", StringComparison.OrdinalIgnoreCase) >= 0 ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;
	ArchiveSuffixFormat = archiveNumbering.Contains("date", StringComparison.OrdinalIgnoreCase) ? _legacyDateArchiveSuffixFormat : _legacySequenceArchiveSuffixFormat;

Conversation

snakefoot commented Dec 17, 2025

Uh oh!

coderabbitai bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Poem

Pre-merge checks and finishing touches

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Dec 17, 2025

Quality Gate passed

Uh oh!

Uh oh!

lechu445 Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

snakefoot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

lechu445 Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

snakefoot Dec 19, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai bot commented Dec 17, 2025 •

edited

Loading