Simplify IsNavigationProperty to reduce cyclomatic complexity#38
Conversation
Codacy flagged the sequential if-statement chain (complexity 15). Collapses the primitive/enum/well-known-scalar-type checks into a single condition backed by a HashSet<Type> lookup, and drops the now-redundant "!= typeof(string)" guard on the collection check (string is already excluded by the scalar-type set by that point). Pure internal simplification, no behavior change - full suite green (786/786), including all scalar/navigation classification tests.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | -9 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
This PR successfully reduces the cyclomatic complexity of IsNavigationProperty by 9 points, meeting the primary architectural goal. The consolidation of type checks into a HashSet improves readability and maintainability. Codacy analysis indicates the changes are up to standards. However, the PR lacks accompanying unit tests to verify that the logic consolidation maintains identical behavior for all edge cases (e.g., nullable types, enums, and byte arrays). While the code appears correct, the lack of test verification for a core logic refactor is the primary concern.
About this PR
- The PR diff does not include tests for the refactored logic. Given that this refactor consolidates multiple conditional branches into a single lookup, it is critical to verify that primitive types, enums, nullable types, and specifically
byte[]continue to be identified correctly as scalars rather than navigation properties.
Test suggestions
- Verify primitive types (e.g., int, bool) are identified as scalar (return false)
- Verify well-known scalar types (string, Guid, DateTime, decimal) are identified as scalar (return false)
- Verify enums are identified as scalar (return false)
- Verify byte arrays (byte[]) are identified as scalar (return false)
- Verify nullable versions of scalar types (e.g., decimal?) are identified as scalar (return false)
- Verify collection types (IEnumerable, List) are identified as navigation properties (return true)
- Verify custom entity classes are identified as navigation properties (return true)
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify primitive types (e.g., int, bool) are identified as scalar (return false)
2. Verify well-known scalar types (string, Guid, DateTime, decimal) are identified as scalar (return false)
3. Verify enums are identified as scalar (return false)
4. Verify byte arrays (byte[]) are identified as scalar (return false)
5. Verify nullable versions of scalar types (e.g., decimal?) are identified as scalar (return false)
6. Verify collection types (IEnumerable, List<T>) are identified as navigation properties (return true)
7. Verify custom entity classes are identified as navigation properties (return true)
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| // Collections of entities are navigation properties (but not string which is IEnumerable<char>) | ||
| if (typeof(System.Collections.IEnumerable).IsAssignableFrom(propertyType) && | ||
| propertyType != typeof(string)) | ||
| // Collections are navigation properties (string, the one scalar IEnumerable, is already handled above) |
There was a problem hiding this comment.
⚪ LOW RISK
Nitpick: The comment mentions only string, but byte[] is also a scalar IEnumerable now handled by the _scalarTypes check at line 156.
| // Collections are navigation properties (string, the one scalar IEnumerable, is already handled above) | |
| // Collections are navigation properties (string and byte[], which are scalar IEnumerables, are already handled above) |
…gationproperty-complexity # Conflicts: # PanoramicData.OData.Client/MemberPathResolver.cs
Summary
Codacy flagged
MemberPathResolver.IsNavigationProperty(added in #36) for high cyclomatic complexity (15), driven by a long sequential if-statement chain: primitive check, an 8-way||for well-known scalar types, enum check,byte[]check, then the collection/class fallback.Collapses the primitive/enum/well-known-scalar-type checks into one condition backed by a
HashSet<Type>lookup, and drops the now-redundant!= typeof(string)guard on the collection check (string is already excluded by the scalar-type set by that point in the method).Pure internal simplification -
IsNavigationPropertyisinternal, not part of the public API surface. No behavior change.Verified
Expand_StringProperty_IsIdentifiedAsScalar,Expand_IntProperty_IsIdentifiedAsScalar,Expand_CollectionProperty_IsIdentifiedAsNavigation, etc.) and allNestedExpandExpressionTests/ExpandWithSelectTests.🤖 Generated with Claude Code