PropertiesDictionary - Simplify enumerator when mixing property types by snakefoot · Pull Request #6061 · NLog/NLog

snakefoot · 2025-12-14T09:20:36Z

No longer attempt to guarantee property ordering, when overriding message-template-properties with additional properties.

By default the underlying Dictionary will try to keep the insert ordering to some point.

coderabbitai · 2025-12-14T09:20:48Z

Walkthrough

Replaces nullable index state with a non-nullable integer sentinel in PropertiesDictionary, simplifying enumerator state/init, MoveNext, Current* accessors and Reset; updates unit test iteration assertions and replaces switch-based traversal with key/value conditional checks.

Changes

Cohort / File(s)	Summary
Internal Enumerator Refactoring `src/NLog/Internal/PropertiesDictionary.cs`	Replaced nullable `_messagePropertiesIndex` with non-nullable `int` sentinel; changed initialization to compute sentinel based on presence of message and event properties; unified boundary checks (`_messagePropertiesIndex >= 0`) across `Current`, `CurrentParameter`, `CurrentProperty`; simplified `MoveNext` to advance message-properties index or fall back to event enumerator; updated `Reset` to reinitialize index and enumerator with new sentinel logic; removed some helper branching.
Test Updates `tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs`	Replaced switch/case index-driven iteration with conditional key/value checks using a `HashSet` to record seen keys for dictionary, `Keys`, and `Values` iterations; updated assertions and counts to reflect new traversal checks and a different final count logic.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Inspect sentinel initialization to ensure all edge cases (no message props, only event props, both present) behave identically to previous logic.
Verify MoveNext path and that fallback to event enumerator retains previous ordering and termination behavior.
Confirm Current* accessors return correct items after refactor and that Reset restores original iteration state.
Review updated tests to ensure they validate equivalent behavior and appropriate counts.

Poem

🐰 I hopped through code with careful paws,
A nullable gone — now integer laws.
Loops simplified, the checks align,
Keys counted true, the tests feel fine.
Tiny hops, big tidy change — hooray! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately reflects the main change: simplifying the enumerator logic in PropertiesDictionary by removing ordered property type mixing guarantees.
Description check	✅ Passed	The description is related to the changeset, explaining that the PR removes property ordering guarantees when mixing message-template-properties with additional properties.

✨ 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: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0418609 and 22dd68c.

📒 Files selected for processing (2)

src/NLog/Internal/PropertiesDictionary.cs (2 hunks)
tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

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

src/NLog/LogEventInfo.cs (2)

PropertiesDictionary (414-429)

PropertiesDictionary (431-445)

src/NLog/Internal/ObjectReflectionCache.cs (19)

Dictionary (558-570)

PropertyValue (294-299)

PropertyValue (301-313)

PropertyValue (315-327)

Enumerator (426-433)

Enumerator (446-453)

Enumerator (455-462)

IEnumerator (435-435)

IEnumerator (436-436)

IEnumerator (656-656)

IEnumerator (668-671)

IEnumerator (673-682)

IEnumerator (684-688)

IEnumerator (722-738)

IEnumerator (744-759)

IEnumerator (761-765)

IEnumerator (768-772)

IEnumerator (784-784)

IEnumerator (791-794)

tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1)

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

Equals (715-715)

Equals (717-725)

Equals (739-742)

Equals (753-767)

Add (214-217)

Add (220-223)

Add (559-559)

Add (644-644)

Clear (226-232)

Clear (562-562)

Clear (647-647)

ToString (732-732)

⏰ 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

🔇 Additional comments (7)

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

449-456: LGTM – Sentinel pattern simplifies state management.

The change from nullable int? to non-nullable int with sentinel values (-1 for message-property iteration, int.MinValue for event-property mode) eliminates nullable checks and clarifies the enumerator's two distinct modes. The initialization logic correctly ensures that message-property mode is only activated when _messageProperties has elements and _eventProperties is null.

462-471: LGTM – Boundary check is safe and consistent with sentinel pattern.

Replacing HasValue check with _messagePropertiesIndex >= 0 correctly identifies when we're at a valid message property index. The array access is safe because MoveNext (line 513) verifies count before incrementing the index from -1 to 0.

478-487: LGTM – CurrentParameter uses same safe boundary check.

Consistent with the Current property changes.

494-504: LGTM – CurrentProperty uses same safe boundary check.

Consistent with the Current property changes.

511-527: LGTM – MoveNext simplified by eliminating mode switching.

The new logic operates in one of two exclusive modes determined at initialization, removing the complexity of transitioning from message properties to event properties mid-iteration. The null-conditional operator at line 513 (_messageProperties?.Count) safely handles the case where _messageProperties is null by making the comparison int < null evaluate to false, preventing invalid array access.

537-538: LGTM – Reset correctly reinitializes state.

The logic mirrors the constructor initialization, ensuring consistent behavior on enumerator reset.

tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1)

372-427: LGTM – Test correctly validates order-independent behavior.

The refactoring from index-based switch statements to HashSet-based key validation appropriately reflects the PR objective: "stops attempting to guarantee property ordering." The new test structure validates:

Both expected keys are present exactly once

Values are correct for each key

No unexpected keys are encountered

This approach is more robust and aligns with the simplified enumerator logic.

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

♻️ Duplicate comments (1)

tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1)

418-425: Same unconventional increment pattern as discussed in the previous comment (lines 375-412).

🧹 Nitpick comments (1)

tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1)

375-412: Consider standard iteration counter for clarity.

The test correctly verifies that both dictionary items are visited by using distinct increments (i += 2 and i += 3, totaling 5). While functionally correct, this pattern is unconventional compared to standard iteration counters (i++ expecting i == 2).

For improved maintainability, consider using either:

Sequential counters with order-dependent assertions, or

A HashSet<object> to track visited keys/values order-independently

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4dd16e8 and 0418609.

📒 Files selected for processing (2)

src/NLog/Internal/PropertiesDictionary.cs (2 hunks)
tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

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

src/NLog/MessageTemplates/MessageTemplateParameter.cs (2)

MessageTemplateParameter (105-111)

MessageTemplateParameter (120-126)

tests/NLog.UnitTests/Internal/PropertiesDictionaryTests.cs (1)

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

Equals (715-715)

Equals (717-725)

Equals (739-742)

Equals (753-767)

🔇 Additional comments (4)

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

449-456: LGTM! Clean sentinel-based state tracking.

The refactor from nullable int? to non-nullable int with sentinel values (-1 for message properties pre-start, int.MinValue for event enumerator mode) eliminates nullable overhead while maintaining clear state semantics. This aligns well with the performance goal of the PR.

458-505: LGTM! Unified boundary check simplifies property accessors.

The consistent use of _messagePropertiesIndex >= 0 across Current, CurrentParameter, and CurrentProperty provides a clean single-condition check to determine whether to read from the message properties list or delegate to the event enumerator.

509-528: LGTM! MoveNext logic is correct and simplified.

The refactored logic cleanly handles two mutually exclusive enumeration modes:

Message properties mode (index >= -1): Advances through the message properties list until exhausted

Event enumerator mode (index < -1): Delegates to the event dictionary enumerator

The boundary check at line 513 safely handles null via the null-conditional operator, returning false when the message properties collection is null or empty.

535-539: LGTM! Reset correctly reinitializes state.

The Reset method properly restores the enumerator to its initial state using the same sentinel initialization logic as the constructor, ensuring consistent behavior for repeated enumerations.

sonarqubecloud · 2025-12-14T10:38:42Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

snakefoot added the performance label Dec 14, 2025

pull-request-size bot added the size/L label Dec 14, 2025

coderabbitai bot reviewed Dec 14, 2025

View reviewed changes

PropertiesDictionary - Simplify enumerator when mixing property types

22dd68c

snakefoot force-pushed the SkipSortOrder branch from 0418609 to 22dd68c Compare December 14, 2025 10:02

snakefoot merged commit b65fffc into NLog:dev Jan 31, 2026
4 of 6 checks passed

snakefoot added this to the 6.1.0 milestone Jan 31, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PropertiesDictionary - Simplify enumerator when mixing property types#6061

PropertiesDictionary - Simplify enumerator when mixing property types#6061
snakefoot merged 1 commit intoNLog:devfrom
snakefoot:SkipSortOrder

snakefoot commented Dec 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 14, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

sonarqubecloud bot commented Dec 14, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

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

Uh oh!

Uh oh!

coderabbitai bot commented Dec 14, 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 14, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

snakefoot commented Dec 14, 2025 •

edited

Loading

coderabbitai bot commented Dec 14, 2025 •

edited

Loading