⚡ various optimizations #8
Conversation
…key extraction for collections and dictionaries
|
Warning Rate limit exceeded@techouse has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 40 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughRefactors internal encode/decode/utils for performance and determinism: single-pass splitting and integrated limit checks in Decoder, sequence caching and fast-paths in Encoder, non-recursive identity-preserving deep string-key conversion in Utils, micro-allocation reductions, and small runtime helpers. No public API signature changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Qs
participant Decoder
participant Utils
Client->>Qs: Decode(query, options)
Qs->>Decoder: ParseQueryStringValues(query, options)
Decoder->>Decoder: Clean input -> split once (allParts/allPartsSeq)
Decoder->>Decoder: Enforce limit via count loop or take allParts
Decoder->>Decoder: Build parts, handle duplicates via TryGetValue + Duplicates switch
Decoder-->>Qs: parts dictionary
Qs->>Utils: ToStringKeyDeepNonRecursive(parts)
Utils-->>Qs: string-keyed object preserving identity
Qs-->>Client: Decoded object
sequenceDiagram
participant Client
participant Qs
participant Encoder
Client->>Qs: Encode(obj, options)
Qs->>Encoder: Encode(obj, options)
Encoder->>Encoder: Detect/materialize sequences (seqList) if IEnumerable
Encoder->>Encoder: Build objKeys via fast-paths (IDictionary/Array/IList/seqList)
Encoder->>Encoder: If comma-gen -> Join seqList into single string
Encoder->>Encoder: Encode values with formatter (IdentityFormatter default)
Encoder-->>Qs: Encoded components
Qs-->>Client: Query string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8 +/- ##
=======================================
Coverage ? 90.25%
=======================================
Files ? 15
Lines ? 1242
Branches ? 349
=======================================
Hits ? 1121
Misses ? 121
Partials ? 0
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
QsNet/Internal/Encoder.cs (1)
159-165: Consider extracting string conversion logicThe string conversion logic here could be extracted into a helper method for better readability and reusability.
Consider extracting the string conversion to a helper method:
+private static List<string> ConvertToStringList(IEnumerable enumerable, bool encodeValuesOnly, ValueEncoder? encoder) +{ + var strings = new List<string>(); + if (encodeValuesOnly && encoder != null) + strings.AddRange(from object? el in enumerable + select el is null ? "" : encoder(el.ToString(), null, null)); + else + strings.AddRange(from object? el in enumerable select el?.ToString() ?? ""); + return strings; +} List<string> strings = []; -if (encodeValuesOnly && encoder != null) - strings.AddRange(from object? el in enumerable - select el is null ? "" : encoder(el.ToString(), null, null)); -else - strings.AddRange(from object? el in enumerable select el?.ToString() ?? ""); +strings = ConvertToStringList(enumerable, encodeValuesOnly, encoder);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
QsNet/Internal/Decoder.cs(2 hunks)QsNet/Internal/Encoder.cs(7 hunks)QsNet/Internal/Utils.cs(10 hunks)QsNet/Qs.cs(8 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
QsNet/Qs.cs (1)
QsNet/Internal/Utils.cs (8)
Dictionary(524-665)Dictionary(848-854)Dictionary(861-870)Dictionary(877-889)Dictionary(942-945)Dictionary(947-1013)Dictionary(1042-1136)List(673-701)
QsNet/Internal/Decoder.cs (3)
QsNet/Internal/Utils.cs (9)
List(673-701)Dictionary(524-665)Dictionary(848-854)Dictionary(861-870)Dictionary(877-889)Dictionary(942-945)Dictionary(947-1013)Dictionary(1042-1136)Utils(20-1137)QsNet/Qs.cs (1)
Dictionary(29-116)QsNet/Extensions.cs (1)
Dictionary(21-27)
QsNet/Internal/Encoder.cs (2)
QsNet/Enums/Format.cs (1)
Formatter(38-46)QsNet/Internal/Decoder.cs (1)
List(387-443)
QsNet/Internal/Utils.cs (3)
QsNet/Models/Undefined.cs (4)
Undefined(6-37)Undefined(11-13)Undefined(33-36)ToString(24-27)QsNet/Qs.cs (1)
Dictionary(29-116)QsNet/Extensions.cs (1)
Dictionary(21-27)
⏰ 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). (4)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (32)
QsNet/Internal/Utils.cs (13)
135-136: Optimize dictionary lookup with single TryGetValue callGood optimization replacing
ContainsKeyfollowed by index access with a singleTryGetValuecall, reducing dictionary lookups from 2 to 1.
149-163: Efficient capacity-aware collection constructionGreat optimization! The changes avoid LINQ overhead and pre-allocate collections with appropriate capacity:
HashSetcreation with source collection avoids resizing- Pre-sized
ListwithtargetList.Count + srcList.Countprevents reallocations during element addition- Direct iteration with conditional checks is more efficient than
Where().Concat()chains
168-176: Consistent capacity optimization for primitive source mergeGood continuation of the capacity-aware pattern. Pre-allocating with
targetList.Count + 1ensures no reallocation when appending the primitive source value.
210-215: Efficient list construction for primitive target with enumerable sourceThe refactored code avoids LINQ overhead by directly building the list with appropriate filtering of
Undefinedvalues in a single pass.
711-724: Cleaner Apply implementation with standard switchGood refactor from pattern switch to standard switch. The explicit list building for
IEnumerable<T>avoids LINQ'sSelect().ToList()overhead.
758-779: Efficient non-allocating HasAny helperExcellent addition! The
HasAnyhelper properly handles disposal of the enumerator and avoids the allocation overhead ofCast<object>().Any(). The explicit disposal pattern ensures proper resource cleanup for enumerators that implementIDisposable.
850-850: Capacity optimization for ToObjectKeyedDictionaryGood performance improvement by pre-allocating the dictionary with
src.Countcapacity, avoiding resize operations during population.
915-918: Safe enumeration pattern for dictionary mutationSmart approach! Copying keys to an array before enumeration prevents potential
InvalidOperationExceptionwhen the dictionary is modified during iteration. This is crucial for the recursive value conversion that follows.
928-930: Direct list construction for sequence conversionGood optimization replacing LINQ's
Cast().Select().ToList()with a direct loop that builds the list. This reduces allocations and improves performance.
1025-1027: More efficient self-reference detectionNice optimization! Iterating
DictionaryEntrydirectly avoids the overhead ofKeys.Cast<object>().Any()and provides direct access to both key and value for the reference check.
1030-1031: Direct DictionaryEntry iteration for copyingConsistent with the self-reference check above, this uses direct
DictionaryEntryiteration for better performance when copying the dictionary.
1058-1132: Well-designed non-recursive deep conversion with identity preservationExcellent implementation! The stack-based approach with visited tracking:
- Avoids stack overflow risks from deep recursion
- Preserves object identity for circular references
- Optimizes for already string-keyed dictionaries by preserving them as-is
- Properly tracks visited nodes to prevent infinite loops
Key improvements:
- Lines 1068-1074: Smart optimization to preserve existing
Dictionary<string, object?>instances- Lines 1076-1086: Proper identity preservation for circular references
- Lines 1104-1108: Consistent identity preservation for nested string-keyed dictionaries in lists
1140-1157: LGTM! ReferenceEqualityComparer implementationClean implementation of a reference equality comparer using
RuntimeHelpers.GetHashCodefor proper reference-based hashing. The singleton pattern with private constructor is appropriate here.QsNet/Internal/Decoder.cs (3)
113-131: Excellent optimization of query string splitting and limit enforcementGreat improvements:
- Single-pass splitting with array materialization avoids multiple enumeration
- Pre-sized list allocation with exact capacity reduces memory allocations
- Early limit enforcement with explicit loop provides better control flow
- Consistent error message for limit violations
The refactored code is cleaner and more performant than the previous
Take().ToList()approach.
134-134: Smart dictionary pre-allocationGood optimization to initialize the dictionary with
parts.Countcapacity, preventing resize operations during population.
212-227: Clean and efficient duplicate handling refactorExcellent refactoring! The new implementation:
- Uses
TryGetValuefor a single dictionary lookup instead ofContainsKey+ indexer- Clear switch statement makes the duplicate handling strategy explicit and maintainable
- Properly handles all cases with appropriate comments
This is more readable and performant than the previous tuple switch approach.
QsNet/Qs.cs (7)
62-62: Consistent dictionary capacity optimizationGood pre-allocation of the dictionary with
tempObj.Countcapacity, aligning with the optimization pattern seen throughout the PR.
140-140: Well-designed local helper function for index dictionary creationExcellent refactoring! The
CreateIndexDictionaryhelper:
- Provides a centralized, reusable solution for converting enumerables to index-keyed dictionaries
- Smart capacity heuristic using
ICollection.Countwhen available- Avoids the overhead of the previous
Cast().Select()LINQ chain- Clear, focused implementation as a static local function
Also applies to: 269-277
178-183: Efficient manual key collectionGood optimization replacing
Cast<object?>().ToList()with manual list construction. Pre-allocating withobj.Countcapacity prevents resizing during population.
193-193: Pre-sized parts list for better memory efficiencyConsistent with the optimization pattern - pre-allocating the parts list with
objKeys.Countcapacity.
202-202: Single dictionary lookup optimizationGood use of
TryGetValueto replace separateContainsKeyand indexer calls, reducing lookups from potentially 2 to 1.
235-240: Direct enumeration for better performanceClean refactoring that avoids LINQ overhead by directly iterating and building the parts list with null checks.
250-250: StringBuilder capacity optimizationSmart pre-sizing of StringBuilder with
joined.Length + 16to accommodate the query prefix and other potential additions, reducing buffer reallocations.QsNet/Internal/Encoder.cs (9)
16-16: Smart identity formatter optimizationExcellent optimization! Creating a static readonly
IdentityFormatterfield eliminates per-call lambda allocations for the default formatter case. This is a simple but effective performance improvement.Also applies to: 65-65
147-154: Effective sequence caching strategyGood design! Materializing sequences once into
seqListprovides:
- Single enumeration of potentially expensive sequences
- Indexed access for better performance
- Accurate count information without re-enumeration
- Consistent behavior across the encoding logic
188-225: Excellent type-specific optimization for key extractionGreat refactoring! The explicit switch statement with type-specific handling:
- Fast path for dictionaries with direct
Keys.Cast<object?>().ToList()- Optimized array/list handling with pre-sized lists and for-loops instead of LINQ
- Proper fallback using cached
seqListwhen available- Clear, maintainable structure
This is much more efficient than the previous generic approach.
231-231: Smart capacity management for values listGood optimization to ensure the values list has sufficient capacity upfront, preventing reallocations during population.
235-235: Sequence cache utilization for array detectionNice use of the cached
seqListto check for single-element arrays without re-enumeration.
239-240: Consistent use of sequence cache for empty checkGood consistency in using the cached
seqListfor the empty check, avoiding re-enumeration.
255-295: Comprehensive dictionary value retrieval optimizationExcellent optimization with type-specific fast paths:
- Dedicated handling for
IDictionary<object, object?>andIDictionary<string, object?>withTryGetValue- Proper fallback for non-generic dictionaries using
Containscheck- Clear undefined value tracking
This reduces dictionary lookups and improves performance for common dictionary types.
344-344: Smart reuse of cached sequence listGood optimization to reuse the cached
seqListinstead of re-materializing the enumerable.Also applies to: 347-348
371-373: Efficient key encoding with guarded string operationSmart optimization to check for '.' presence before calling
Replace, avoiding unnecessary string allocations when dots aren't present.
…cally to reduce allocations
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
QsNet/Internal/Encoder.cs (1)
69-71: Bug: ReferenceEquals on delegates will mis-identify the comma generatorDelegates created by ListFormat.Comma.GetGenerator() are distinct instances; ReferenceEquals will be false even when they refer to the same method. This likely disables comma-encoding behavior unless commaRoundTrip is explicitly set.
Use delegate equality (==) or compare Method/Target to identify the comma generator:
- var isCommaGen = ReferenceEquals(gen, ListFormat.Comma.GetGenerator()); + var isCommaGen = gen == ListFormat.Comma.GetGenerator(); + // Alternatively, if you want to avoid an extra delegate allocation: + // var commaGen = ListFormat.Comma.GetGenerator(); + // var isCommaGen = gen.Method == commaGen.Method && Equals(gen.Target, commaGen.Target);If feasible, exposing an IsComma flag alongside the generator (or passing ListFormat instead of the generator) would make this robust and allocation-free.
🧹 Nitpick comments (5)
QsNet/Internal/SideChannelFrame.cs (2)
29-36: Set now updates in-place when key exists — good; ensure intended single-threaded useUpdating Box.Value when the key already exists avoids a reallocation and extra table mutation. Given ConditionalWeakTable is thread-safe but Box.Value is not synchronized, this is fine as long as SideChannelFrame instances are not shared across threads for the same encode/encode operation. If a shared SideChannelFrame instance can be used concurrently, consider documenting it or marking Value as volatile (only if needed).
47-48: Box.Value made mutable enables in-place updatesThe mutability change is required to support Set’s fast-path. Keep in mind: Box is internal, so exposing a public field is acceptable here. If SideChannelFrame ever becomes concurrently accessed, a volatile or property with interlocked semantics might be warranted, but likely unnecessary now.
QsNet/Constants/HexTable.cs (1)
13-25: Optional: avoid per-entry char[] allocation with string.Create on non-NETSTANDARD2_0This micro-optimization trims allocations in the static initializer while keeping the same semantics.
Apply under a conditional to preserve NETSTANDARD2_0 compatibility:
- private static string[] Create() + private static string[] Create() { var arr = new string[256]; for (var i = 0; i < 256; i++) { - var chars = new char[3]; - chars[0] = '%'; - chars[1] = GetHexChar((i >> 4) & 0xF); - chars[2] = GetHexChar(i & 0xF); - arr[i] = new string(chars); +#if NETSTANDARD2_0 + var chars = new char[3]; + chars[0] = '%'; + chars[1] = GetHexChar((i >> 4) & 0xF); + chars[2] = GetHexChar(i & 0xF); + arr[i] = new string(chars); +#else + arr[i] = string.Create(3, i, static (span, val) => + { + span[0] = '%'; + span[1] = GetHexChar((val >> 4) & 0xF); + span[2] = GetHexChar(val & 0xF); + }); +#endif } return arr; } - private static char GetHexChar(int n) + private static char GetHexChar(int n) { return (char)(n < 10 ? ('0' + n) : ('A' + (n - 10))); }Also applies to: 27-30
QsNet/Internal/Decoder.cs (2)
253-264: Parent key aggregation avoids string.Join — fine; tiny readability tweak optionalThe StringBuilder loop is allocation-friendly. If desired, you can early-return string.Empty when chain.Count == 1 to avoid constructing the builder in that branch, but current code already does that.
491-513: Helper JoinAsCommaSeparatedStrings is fine; consider a fast-path for empty enumerablesMinor micro-optimization: peek the first element before allocating the StringBuilder to avoid creating it for empty enumerables. Not critical.
Apply:
- private static string JoinAsCommaSeparatedStrings(IEnumerable enumerable) + private static string JoinAsCommaSeparatedStrings(IEnumerable enumerable) { - var e = enumerable.GetEnumerator(); - var sb = new StringBuilder(); - var first = true; + var e = enumerable.GetEnumerator(); + StringBuilder? sb = null; + var first = true; try { while (e.MoveNext()) { - if (!first) sb.Append(','); - var s = e.Current?.ToString() ?? string.Empty; - sb.Append(s); + if (first) + { + sb = new StringBuilder(); + first = false; + } + else + { + sb!.Append(','); + } + var s = e.Current?.ToString() ?? string.Empty; + sb!.Append(s); - first = false; } } finally { (e as IDisposable)?.Dispose(); } - return sb.ToString(); + return sb?.ToString() ?? string.Empty; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
QsNet/Constants/HexTable.cs(1 hunks)QsNet/Internal/Decoder.cs(7 hunks)QsNet/Internal/Encoder.cs(8 hunks)QsNet/Internal/SideChannelFrame.cs(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
QsNet/Internal/Encoder.cs (4)
QsNet/Enums/Format.cs (1)
Formatter(38-46)QsNet/Internal/Decoder.cs (1)
List(406-462)QsNet/Enums/Sentinel.cs (1)
ToString(65-68)QsNet/Models/Undefined.cs (1)
ToString(24-27)
⏰ 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). (4)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (19)
QsNet/Internal/SideChannelFrame.cs (1)
16-27: Inlining TryGet is appropriate hereAggressiveInlining on this tiny hot-path accessor is a good fit.
QsNet/Constants/HexTable.cs (1)
11-12: Lazy-initializing the table via Create() is fineNo functional change; the runtime init keeps the public surface intact.
QsNet/Internal/Decoder.cs (8)
113-121: Single-pass split + pre-sizing are solid winsReusing allParts and pre-sizing parts/obj avoid redundant allocations and repeated splitting. The early throw on limit exceeded is also clearer and avoids post-work checks.
Also applies to: 130-135
140-151: Deterministic charset sentinel detection via ordinal StartsWithUsing StringComparison.Ordinal for the utf8 sentinel is correct and avoids culture pitfalls.
198-204: Numeric-entity interpretation: safe conversion path for enumerablesJoining non-string enumerables before InterpretNumericEntities avoids LINQ and handles multi-valued inputs. This preserves behavior with a lower allocation profile.
205-207: Double-check semantics for '[]=' when value is already an IEnumerableCurrent logic wraps enumerable values in a single-item List (i.e., List<object?> { value }). For inputs like a[]=1,2 with options.Comma=true, this creates a nested list (one element containing a list), which may differ from expected “flat” list semantics.
Would you like me to add unit tests for cases such as:
- "?a[]=1,2" with Comma=true (expect ["1","2"] vs [[ "1","2" ]])
- "?a[]=1&a[]=2" (expect ["1","2"])?
I can generate them to lock in the intended behavior.
208-224: Cleaner duplicate-handling via TryGetValue + switch is 👍The Combine/Last/First switch reads clearly and avoids a second lookup.
279-289: Manual self-ref detection for maps avoids LINQ.Any — goodThe explicit loop is clearer and cheaper. Preserving identity for self-referential dictionaries is important for correctness.
317-325: Ordinal-insensitive dot decoding across TFMs is consistentThe NETSTANDARD2_0 helper and StringComparison-based replaces align behavior and remove culture sensitivity.
345-350: Pre-sizing list to idx+1 avoids growth churnThis aligns with the ListLimit enforcement and reduces reallocations when building sparse lists with Undefined placeholders.
QsNet/Internal/Encoder.cs (9)
16-16: IdentityFormatter removes per-call lambda allocs — goodUsing a cached identity delegate and defaulting fmt = formatter ?? IdentityFormatter is a tidy way to avoid unnecessary allocations.
Also applies to: 65-65
101-115: Normalization for comma sequences is sensibleNormalizing DateTime/DateTimeOffset before comma-join helps round-trip. The cast/select then materialization is appropriate given later usage.
147-155: Sequence detection and single materialization reduces re-enumerationsCaching seqList and reusing it later for indexing/capacity checks is a solid perf improvement.
159-177: Comma branch now emits a single joined value — confirm intended behaviorSwitching to a single "value" entry with a joined string changes observable output for comma-encoded lists (vs. emitting multiple entries). This looks intentional for round-trip, but it’s worth validating against existing tests and Decoder’s expectations.
Would you like me to add cross-tests that encode with ListFormat.Comma and decode back (with Comma=true/false) to ensure round-trip equivalence for:
- single-element lists
- empty lists (allowEmptyLists both true/false)
- nested enumerables?
Also applies to: 236-241
189-226: Type-specific fast paths (IDictionary/Array/IList/IEnumerable) look correct
- Pre-sized index lists avoid growth.
- IDictionary paths with TryGetValue reduce lookups and boxing.
- IEnumerable branch uses seqList when available and a uint-bounds check — nice touch.
Also applies to: 249-297, 339-350
232-233: values.Capacity pre-grow avoids reallocationsGood allocation hygiene.
371-375: Guarded dot-encoding for keys avoids unnecessary Replace allocationsChecking IndexOf('.') before Replace is a cheap win.
383-387: Side-channel step bookkeeping is correct; child frame per recursion prevents aliasingSetting step before recursing and creating a new SideChannelFrame per child aligns with the TryGet/Set logic added in SideChannelFrame.
416-419: Flattening IEnumerable results into the values list is correctThe outer Encode returns a flat collection when recursing. The guard for string prevents accidental char iteration.
…omparer for improved code clarity
… reduce allocations
…aSeparatedStrings to reduce memory usage
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
QsNet/Internal/Utils.cs (3)
210-216: Pre-size the result list when source IEnumerable has a known CountMinor allocation win: if source is an ICollection, you can pre-size the result list to reduce resizes when appending.
Apply this diff:
- if (source is not IEnumerable<object?> src2) return new List<object?> { target, source }; - var list = new List<object?> { target }; + if (source is not IEnumerable<object?> src2) return new List<object?> { target, source }; + var cap = 1; + if (src2 is System.Collections.ICollection c) cap += c.Count; + var list = new List<object?>(cap) { target }; foreach (var v in src2) if (v is not Undefined) list.Add(v); return list;
711-724: Apply: pre-size when possible to reduce allocationsGreat removal of LINQ allocations. You can also pre-size when the enumerable exposes a Count.
Apply this diff:
- case IEnumerable<T> enumerable: - { - var list = new List<T>(); + case IEnumerable<T> enumerable: + { + var list = enumerable is ICollection<T> coll ? new List<T>(coll.Count) : new List<T>(); foreach (var it in enumerable) list.Add(fn(it)); return list; }
929-931: Pre-size seqList based on ICollection to reduce reallocationsWhen the source sequence exposes a Count, pre-sizing reduces growth copies while building the list.
Apply this diff:
- var seqList = new List<object?>(); + var seqList = seq is System.Collections.ICollection c ? new List<object?>(c.Count) : new List<object?>(); foreach (var v in seq) seqList.Add(ConvertNestedValues(v, visited)); return seqList;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
QsNet/Internal/Utils.cs(11 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
QsNet/Internal/Utils.cs (3)
QsNet/Models/Undefined.cs (4)
Undefined(6-37)Undefined(11-13)Undefined(33-36)ToString(24-27)QsNet/Internal/Decoder.cs (2)
List(406-462)Dictionary(91-227)QsNet/Qs.cs (1)
Dictionary(29-116)
⏰ 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). (4)
- GitHub Check: Coverage (merged)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Codacy Security Scan
- GitHub Check: Analyze (csharp)
🔇 Additional comments (10)
QsNet/Internal/Utils.cs (10)
135-141: Index-merge path uses TryGetValue to avoid double lookups — good improvementSwitching to TryGetValue when merging index-based maps avoids two dictionary lookups and keeps value retrieval and merge atomic. This also clarifies intent.
150-156: Set fallback: one-pass append with Undefined filtering — LGTMUsing a HashSet initialized from target and appending non-Undefined items from source is clear and avoids repeated enumeration.
158-163: List fallback: pre-sized concat with Undefined filtering — LGTMPre-sizing the result list and filtering Undefined during append reduces allocations and keeps semantics explicit.
168-176: Primitive append to set/list fast-path — LGTMDirectly materializing sets/lists for primitive sources avoids unnecessary branching and repeated enumeration.
758-759: Replace Cast/Any with HasAny — LGTMCheaper emptiness check without LINQ allocations. This benefits hot paths in IsEmpty.
763-779: HasAny implementation is safe and minimal — LGTMManual enumerator usage with try/finally disposal avoids interface-boxing overhead from LINQ and is compatible with non-disposable enumerators.
850-854: ToObjectKeyedDictionary pre-sizes to src.Count — LGTMGood micro-optimization to reduce rehashing for common conversions from non-generic maps.
915-919: ConvertNestedValues: copy keys before mutating — LGTMCopying keys to an array before in-place value conversion prevents invalidating the enumerator and is the correct way to handle mutation during traversal.
947-953: Doc comment alignment onlyNo functional changes here.
1031-1038: NormalizeForTarget: robust self-reference check and explicit copy — LGTMSwitching to DictionaryEntry iteration for self-reference detection and performing an explicit copy avoids key-casting overhead and handles non-generic IDictionary correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
QsNet/Constants/HexTable.cs (2)
15-35: Cross-target generation is sound; consider tiny readability/micro-alloc tweaksThe NETSTANDARD2_0 fallback and string.Create fast-path are correct and allocation-efficient. Two small optional improvements:
- Use arr.Length in the loop to avoid repeating the magic number.
- Replace GetHexChar calls with an index into a constant "0123456789ABCDEF" to remove branching and duplicate logic between branches. This is a one-time init, so it’s a nit; it also slightly simplifies the code.
Apply within this hunk:
- for (var i = 0; i < 256; i++) + for (var i = 0; i < arr.Length; i++) { #if NETSTANDARD2_0 var chars = new char[3]; chars[0] = '%'; - chars[1] = GetHexChar((i >> 4) & 0xF); - chars[2] = GetHexChar(i & 0xF); + chars[1] = HexDigits[(i >> 4) & 0xF]; + chars[2] = HexDigits[i & 0xF]; arr[i] = new string(chars); #else arr[i] = string.Create(3, i, static (span, val) => { span[0] = '%'; - span[1] = GetHexChar((val >> 4) & 0xF); - span[2] = GetHexChar(val & 0xF); + span[1] = HexDigits[(val >> 4) & 0xF]; + span[2] = HexDigits[val & 0xF]; }); #endif }To support the above (outside this hunk), add a shared constant in the class:
// Place near the top of the class private const string HexDigits = "0123456789ABCDEF";
37-40: Optional: Remove GetHexChar in favor of a shared hex-digit lookupGiven this is only used during table initialization, you can drop GetHexChar and use a shared "0123456789ABCDEF" lookup as shown in the previous comment, which reduces branching and keeps both branches consistent.
Apply this diff if you adopt the lookup approach:
- private static char GetHexChar(int n) - { - return (char)(n < 10 ? '0' + n : 'A' + (n - 10)); - } + // Not needed if using HexDigits lookup; safe to remove.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
QsNet/Constants/HexTable.cs(1 hunks)QsNet/Internal/Decoder.cs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- QsNet/Internal/Decoder.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: Codacy Security Scan
🔇 Additional comments (1)
QsNet/Constants/HexTable.cs (1)
11-11: LGTM: Static generation keeps behavior while shrinking IL and avoiding giant literalsInitializing the table via a small helper method is cleaner and avoids the maintenance burden of a 256-entry literal. The public surface stays the same. No concerns here.
…ing traversal for dictionaries and lists
…of nested lists and dictionaries
1 participant
This pull request includes several optimizations and correctness improvements to the query string encoder and decoder modules, as well as a new test for deep conversion of nested lists and dictionaries. The main themes are performance improvements, better handling of edge cases, and code simplification for maintainability and compatibility.
Decoder optimizations and correctness improvements:
QsNet/Internal/Decoder.cs)QsNet/Internal/Decoder.cs) [1] [2] [3]QsNet/Internal/Decoder.cs) [1] [2]QsNet/Internal/Decoder.cs) [1] [2]Encoder performance and correctness improvements:
QsNet/Internal/Encoder.cs) [1] [2] [3]QsNet/Internal/Encoder.cs) [1] [2]General codebase improvements:
QsNet/Constants/HexTable.cs)QsNet.Tests/UtilsTests.cs)Summary by CodeRabbit
Refactor
Chores