handled null cheks in pt-calculator - #385
Conversation
Added multiarch changes
WalkthroughInternal updates to EstimationService’s slab filtering logic add null-safety, defensive defaults, a null-safe string comparator, revised matching rules for area, property type/subtype, ownership, and updated logging; no public method signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant ES as EstimationService
participant D as Property Details
participant S as Slab Config
C->>ES: getSlabsFiltered(details)
ES->>D: Read landArea/buildUpArea, types, ownership, floors
Note right of ES: Apply null-safe defaults and safeEqualsIgnoreCase<br/>derive dtlAreaType, dtlIsMultiFloored
ES->>S: Iterate slabs
loop For each slab
ES->>S: Read slab fields with null-tolerant defaults
alt Area type match
ES-->ES: dtlAreaType empty => match<br/>else match if slabAreaType equals dtlAreaType or "ALL"
else No match
ES-->>ES: Skip slab
end
alt Property/ownership match
ES-->ES: Match on type/subtype/ownership or "ALL"/empty detail
else No match
ES-->>ES: Skip slab
end
ES-->ES: Check plot range (special-case when plotSize == 0.0) and multi-floor flag
ES-->>ES: finalResult = all matches && plotMatch
opt Logging
ES->>ES: Debug log slab when finalResult true or slab id short
end
ES-->>ES: Collect matching slab if finalResult
end
ES-->>C: Return filtered slabs (with post-filter log)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
Status, Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
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)
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java(1 hunks)
🔇 Additional comments (5)
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java (5)
561-570: LGTM: Null-safe extraction of area type.The defensive checks around Address → Locality → Area with a safe default ("") are correct and prevent NPEs. This aligns with the downstream wildcard logic.
571-573: LGTM: Robust multi-floor flag computation.Calculates a definitive boolean for dtlIsMultiFloored with null-guard on noOfFloors; concise and safe.
589-593: LGTM: Area matching honors wildcard and “ALL” sentinel.The logic correctly considers empty detail as wildcard and recognizes slab “ALL”. This prevents false negatives when area is unavailable.
597-611: LGTM: Wildcard semantics for subtype/ownership are consistent and null-safe.Empty detail values act as wildcard; slab “ALL” is honored. This mirrors area-type handling and reduces NPE risk during lookups.
555-560: Ensure wildcard matching forpropertyTypein EstimationServiceCurrently, in EstimationService (around lines 555–560),
dtlPtTypeis defaulted to""when detail.getPropertyType() is null, but the subsequent matcherboolean isPtTypeMatching = !dtlPtType.isEmpty() && slabPropertyType.equalsIgnoreCase(dtlPtType);will reject all slabs whenever
dtlPtTypeis empty—contrary to the null-tolerant, wildcard behavior applied to other fields.Please verify and update as follows:
• In
EstimationService.java(≈ lines 555–560) replace the existing matcher- boolean isPtTypeMatching = !dtlPtType.isEmpty() - && slabPropertyType.equalsIgnoreCase(dtlPtType); + boolean isPtTypeMatching = dtlPtType.isEmpty() + || slabPropertyType.equalsIgnoreCase(dtlPtType) + || slabPropertyType.equalsIgnoreCase(BillingSlabConstants.ALL_PLACEHOLDER_BILLING_SLAB);• Confirm that the matchers for
dtlPtSubType,dtlOwnerShipCat, anddtlSubOwnerShipCatalready handle empty detail fields and the"ALL"sentinel consistently (as seen in BillingSlabUtils.java).• Add focused unit tests to cover:
– Detail’spropertyTypenull/empty → should not filter out slabs.
– Slab’spropertyType = "ALL"→ matches any detail value.Would you like me to draft these unit tests?
Added multiarch changes
# Conflicts: # property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java (2)
621-627: Fix boundary condition: exclude-equal on lower bound causes slab gaps.Using slabAreaFrom < plotSize excludes properties exactly at fromPlotSize (except the 0.0 special case), creating holes. Use inclusive lower bound consistently. Also, if plot size is unknown, don’t restrict by area at all.
- boolean isPlotMatching = false; - - if (plotSize == 0.0) - isPlotMatching = slabAreaFrom <= plotSize && slabAreaTo >= plotSize; - else - isPlotMatching = slabAreaFrom < plotSize && slabAreaTo >= plotSize; + boolean isPlotMatching; + if (!hasPlotSize) { + // No plot size provided; don't restrict by area + isPlotMatching = true; + } else { + // Inclusive lower bound to avoid gaps at exact 'from' values + isPlotMatching = slabAreaFrom <= plotSize && slabAreaTo >= plotSize; + }
551-649: Functional coverage: add tests for boundary and null-safety scenarios in slab filtering.Please add unit tests that pin the intended behavior:
- plotSize unknown (both landArea and buildUpArea null) should not filter by area.
- plotSize exactly equal to fromPlotSize and toPlotSize should match.
- slab.isPropertyMultiFloored null should match both single- and multi-floor properties (subject to domain confirmation).
- Case and whitespace differences in propertyType/subType/ownership fields should not affect matching.
I can scaffold a focused test class for getSlabsFiltered with mocked BillingSlabService returning representative slabs (ALL sentinel, null multi-floor, boundary ranges). Want me to draft it?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
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)
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java(3 hunks)
🔇 Additional comments (1)
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java (1)
584-596: Treat nullisPropertyMultiFlooredas “no restriction” rather thanfalseThe current code defaults a
nullMDMS value tofalse, inadvertently excluding slabs that were meant to apply to both single- and multi-floor properties. To align with the “ALL” semantics used elsewhere, anullflag should not constrain the match.Please confirm with the product/domain team whether a
nullvalue forslab.isPropertyMultiFlooredin MDMS is intended to mean “apply to all properties,” so we can safely adopt this change.Key locations:
- File:
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java- Lines: 584–596
Suggested diff:
- Boolean slabMultiFloored = slab.getIsPropertyMultiFloored() != null ? slab.getIsPropertyMultiFloored() : false; + // Treat null as “no restriction” (apply to both single- and multi-floor) + Boolean slabMultiFloored = slab.getIsPropertyMultiFloored(); @@ - boolean isPropertyMultiFloored = slabMultiFloored.equals(dtlIsMultiFloored); + // If MDMS flag is null, allow both; otherwise, require exact match + boolean isPropertyMultiFloored = (slabMultiFloored == null) + || (slabMultiFloored.booleanValue() == dtlIsMultiFloored);[tag: verify_review_comment]
| /** | ||
| * Safe null-aware case-insensitive string comparison | ||
| * @param str1 First string to compare (can be null) | ||
| * @param str2 Second string to compare (can be null) | ||
| * @return true if both strings are equal (ignoring case), false otherwise | ||
| */ | ||
| private boolean safeEqualsIgnoreCase(String str1, String str2) { | ||
| if (str1 == null && str2 == null) { | ||
| return true; | ||
| } | ||
| if (str1 == null || str2 == null) { | ||
| return false; | ||
| } | ||
| return str1.equalsIgnoreCase(str2); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove custom safeEqualsIgnoreCase; use Apache StringUtils.equalsIgnoreCase instead.
You already depend on commons-lang3; avoid duplicating well-tested utilities and reduce surface area.
- /**
- * Safe null-aware case-insensitive string comparison
- * @param str1 First string to compare (can be null)
- * @param str2 Second string to compare (can be null)
- * @return true if both strings are equal (ignoring case), false otherwise
- */
- private boolean safeEqualsIgnoreCase(String str1, String str2) {
- if (str1 == null && str2 == null) {
- return true;
- }
- if (str1 == null || str2 == null) {
- return false;
- }
- return str1.equalsIgnoreCase(str2);
- }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In
property-tax/pt-calculator-v2/src/main/java/org/egov/pt/calculator/service/EstimationService.java
around lines 1246 to 1260, remove the custom safeEqualsIgnoreCase method and
replace all its call sites with
org.apache.commons.lang3.StringUtils.equalsIgnoreCase(...) from commons-lang3;
add or ensure the import org.apache.commons.lang3.StringUtils; remove the
private method implementation entirely to avoid duplication and rely on the
tested library utility.
2 participants
Added multiarch changes
Summary by CodeRabbit
Bug Fixes
Refactor