Pm cluster old #1475
Pm cluster old #1475
Conversation
…ssue, fixed dimagi logo background
WalkthroughThis update introduces geometric polygon utilities and two new XPath functions: one to determine if a point lies inside a polygon, and another to find the closest point on a polygon boundary to a given geopoint. The changes include new Java utility and function classes, Gradle wrapper scripts for build consistency, and corresponding test cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant XPath Engine
participant PolygonUtils
User->>XPath Engine: Evaluate "inside-polygon(polygon, point)"
XPath Engine->>PolygonUtils: isPointInsidePolygon(polygon, point)
PolygonUtils-->>XPath Engine: true/false
XPath Engine-->>User: Result
User->>XPath Engine: Evaluate "polygon-point(polygon, point)"
XPath Engine->>PolygonUtils: getClosestPoint(polygon, point)
PolygonUtils-->>XPath Engine: closest_point_string
XPath Engine-->>User: Result
Poem
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
gradlew.bat (1)
70-73: Minor formatting issue with empty line.There's an empty line at line 72 between setting the classpath and the "Execute Gradle" section. This appears to be unintentional and could be removed for consistency.
set CLASSPATH=%APP_HOME%\gradle\wrapper\gradle-wrapper.jar - @rem Execute Gradlesrc/main/java/org/javarosa/core/model/utils/PolygonUtils.java (1)
4-14: Note the AI assistance comment in JavaDocThe JavaDoc contains a note indicating that the code was written with the help of ChatGPT. While transparency is good, you might want to:
- Remove this comment since it's not relevant to the API documentation
- Add more details about the algorithm used (ray-casting) instead
* @param testPoint A list of doubles representing the latitude and longitude of the test point. * @return true if the point is inside the polygon, false otherwise. -* This code is written with the help of chatgpt */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (9)
gradle/wrapper/gradle-wrapper.properties(1 hunks)gradlew(1 hunks)gradlew.bat(1 hunks)src/main/java/org/javarosa/core/model/utils/PolygonUtils.java(1 hunks)src/main/java/org/javarosa/xpath/expr/FunctionUtils.java(1 hunks)src/main/java/org/javarosa/xpath/expr/XPathClosestPolygonPointFunc.java(1 hunks)src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java(1 hunks)src/main/java/org/javarosa/xpath/parser/ast/ASTNodeFunctionCall.java(3 hunks)src/test/java/org/javarosa/xpath/test/XPathEvalTest.java(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/main/java/org/javarosa/xpath/expr/FunctionUtils.java (2)
src/main/java/org/javarosa/xpath/expr/XPathClosestPolygonPointFunc.java (1)
XPathClosestPolygonPointFunc(14-61)src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java (1)
XPathPointInsidePolygon(13-60)
src/main/java/org/javarosa/xpath/parser/ast/ASTNodeFunctionCall.java (2)
src/main/java/org/javarosa/xpath/expr/XPathClosestPolygonPointFunc.java (1)
XPathClosestPolygonPointFunc(14-61)src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java (1)
XPathPointInsidePolygon(13-60)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
- GitHub Check: task-list-completed
🔇 Additional comments (16)
gradle/wrapper/gradle-wrapper.properties (1)
1-5:Details
❓ Verification inconclusive
Gradle Wrapper configuration looks good.
The configuration properly sets up the Gradle Wrapper to use version 7.5.1 from the official Gradle distribution service. The paths and configuration are standard for Gradle Wrapper.
🏁 Script executed:
#!/bin/bash # Check if a newer version of Gradle is available curl -s https://services.gradle.org/versions/current | grep -o '"version":"[^"]*"' | cut -d'"' -f4Length of output: 100
Please verify the Gradle Wrapper version manually
The settings in
gradle/wrapper/gradle-wrapper.properties(lines 1–5) correctly point to Gradle 7.5.1 using the standard distribution URL and paths. An automated API check against https://services.gradle.org/versions/current did not return a version in this environment, so please confirm that 7.5.1 is still the intended/latest stable release before merging.gradlew.bat (2)
1-91: Standard Gradle Wrapper script for Windows environments.The script is well-structured with proper error handling for Java detection, appropriate memory allocation settings, and follows standard Gradle Wrapper conventions.
36-36: Consider project's memory requirements for JVM settings.The default JVM settings allocate 64MB for minimum and maximum heap space, which may be insufficient for larger projects.
This is a standard default but check if your project's build requires more memory. For larger projects, consider increasing these values.
gradlew (3)
1-240: Standard Gradle Wrapper shell script for POSIX environments.The script is comprehensive with proper handling for different operating systems, symbolic links resolution, Java detection, and follows standard Gradle Wrapper conventions.
89-89: Consider project's memory requirements for JVM settings.The default JVM settings allocate 64MB for minimum and maximum heap space, which may be insufficient for larger projects.
This is a standard default but check if your project's build requires more memory. For larger projects, consider increasing these values.
1-1:Details
✅ Verification successful
Ensure script has executable permissions.
The script needs executable permissions to function properly on POSIX systems.
🏁 Script executed:
#!/bin/bash # Check if the gradlew script has executable permissions ls -l gradlew # If needed, you can make it executable with: # git update-index --chmod=+x gradlewLength of output: 72
Executable permissions for gradlew confirmed
The
gradlewscript already has executable permissions (-rwxr-xr-x), so no further action is required.src/main/java/org/javarosa/xpath/expr/FunctionUtils.java (1)
93-94: LGTM! New polygon functions have been registered properly.These additions properly register the two new XPath functions
polygon-pointandinside-polygonin the function registry, allowing them to be used in XPath expressions.src/test/java/org/javarosa/xpath/test/XPathEvalTest.java (2)
611-613: Good coverage for the polygon-point function.The test verifies that the function correctly identifies the closest point on a polygon boundary to a given external point.
614-618: Comprehensive test coverage for inside-polygon functionality.The tests cover multiple important scenarios:
- A point inside the polygon
- Points outside the polygon in different locations
- A point exactly on a polygon vertex
This ensures the function works correctly across various edge cases.
src/main/java/org/javarosa/xpath/parser/ast/ASTNodeFunctionCall.java (3)
10-10: LGTM! Import for new XPathClosestPolygonPointFunc class.The import has been correctly added for the new polygon-point function.
52-52: LGTM! Import for new XPathPointInsidePolygon class.The import has been correctly added for the new inside-polygon function.
262-265: LGTM! New polygon functions properly added to the function builder.The implementation correctly adds the two new XPath functions to the function builder switch statement:
polygon-pointmapping toXPathClosestPolygonPointFuncinside-polygonmapping toXPathPointInsidePolygonThis allows the parser to properly recognize and instantiate these functions when they're used in XPath expressions.
src/main/java/org/javarosa/xpath/expr/XPathClosestPolygonPointFunc.java (4)
1-13: LGTM! Appropriate imports and package declaration.The file has all the necessary imports for working with geometric functions and XPath expression evaluation.
14-26: LGTM! Class definition and constructors follow established patterns.The class properly extends
XPathFuncExprwith:
- A static NAME constant matching the XPath function name
- The correct EXPECTED_ARG_COUNT constant set to 2
- Two constructors matching the pattern used by other XPath functions
27-37: LGTM! Good documentation and simple implementation.The function includes:
- Clear documentation of what the function does, including edge cases
- A clean implementation that delegates to a static method for the actual logic
38-60: LGTM! Robust implementation with proper error handling.The implementation:
- Correctly unpacks arguments and handles null/empty values
- Parses the polygon coordinates from a space-separated string
- Has appropriate error handling for invalid numeric inputs
- Uses the
PolygonUtilsutility for the geometric calculations- Returns the closest point in the expected format
The comment on line 51 about casting being strange but consistent with the codebase is helpful for future maintainers.
| private static boolean rayIntersectsEdge(double testLat, double testLng, double lat1, double lng1, double lat2, double lng2) { | ||
| if (lat1 > lat2) { | ||
| double tempLat = lat1, tempLng = lng1; | ||
| lat1 = lat2; | ||
| lng1 = lng2; | ||
| lat2 = tempLat; | ||
| lng2 = tempLng; | ||
| } | ||
|
|
||
| if (testLat < lat1 || testLat > lat2) { | ||
| return false; | ||
| } | ||
|
|
||
| if (testLng > Math.max(lng1, lng2)) { | ||
| return false; | ||
| } | ||
|
|
||
| if (testLng < Math.min(lng1, lng2)) { | ||
| return true; | ||
| } | ||
|
|
||
| double slope = (lng2 - lng1) / (lat2 - lat1); | ||
| double intersectLng = lng1 + (testLat - lat1) * slope; | ||
|
|
||
| return testLng < intersectLng; | ||
| } |
There was a problem hiding this comment.
Fix potential division by zero in rayIntersectsEdge
The rayIntersectsEdge method can throw a division by zero exception if lat2 == lat1 (a horizontal edge in terms of latitude).
private static boolean rayIntersectsEdge(double testLat, double testLng, double lat1, double lng1, double lat2, double lng2) {
if (lat1 > lat2) {
double tempLat = lat1, tempLng = lng1;
lat1 = lat2;
lng1 = lng2;
lat2 = tempLat;
lng2 = tempLng;
}
if (testLat < lat1 || testLat > lat2) {
return false;
}
if (testLng > Math.max(lng1, lng2)) {
return false;
}
if (testLng < Math.min(lng1, lng2)) {
return true;
}
+ // Check for horizontal line to avoid division by zero
+ if (lat2 - lat1 == 0) {
+ return false; // Horizontal line can't intersect with a horizontal ray
+ }
double slope = (lng2 - lng1) / (lat2 - lat1);
double intersectLng = lng1 + (testLat - lat1) * slope;
return testLng < intersectLng;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static boolean rayIntersectsEdge(double testLat, double testLng, double lat1, double lng1, double lat2, double lng2) { | |
| if (lat1 > lat2) { | |
| double tempLat = lat1, tempLng = lng1; | |
| lat1 = lat2; | |
| lng1 = lng2; | |
| lat2 = tempLat; | |
| lng2 = tempLng; | |
| } | |
| if (testLat < lat1 || testLat > lat2) { | |
| return false; | |
| } | |
| if (testLng > Math.max(lng1, lng2)) { | |
| return false; | |
| } | |
| if (testLng < Math.min(lng1, lng2)) { | |
| return true; | |
| } | |
| double slope = (lng2 - lng1) / (lat2 - lat1); | |
| double intersectLng = lng1 + (testLat - lat1) * slope; | |
| return testLng < intersectLng; | |
| } | |
| private static boolean rayIntersectsEdge(double testLat, double testLng, double lat1, double lng1, double lat2, double lng2) { | |
| if (lat1 > lat2) { | |
| double tempLat = lat1, tempLng = lng1; | |
| lat1 = lat2; | |
| lng1 = lng2; | |
| lat2 = tempLat; | |
| lng2 = tempLng; | |
| } | |
| if (testLat < lat1 || testLat > lat2) { | |
| return false; | |
| } | |
| if (testLng > Math.max(lng1, lng2)) { | |
| return false; | |
| } | |
| if (testLng < Math.min(lng1, lng2)) { | |
| return true; | |
| } | |
| // Check for horizontal line to avoid division by zero | |
| if (lat2 - lat1 == 0) { | |
| return false; // Horizontal line can't intersect with a horizontal ray | |
| } | |
| double slope = (lng2 - lng1) / (lat2 - lat1); | |
| double intersectLng = lng1 + (testLat - lat1) * slope; | |
| return testLng < intersectLng; | |
| } |
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/core/model/utils/PolygonUtils.java between lines
98 and 123, the method rayIntersectsEdge can cause a division by zero error when
lat2 equals lat1. To fix this, add a check before calculating the slope to
handle the case when lat2 == lat1, such as returning false immediately or
handling the horizontal edge appropriately to avoid dividing by zero.
| private static double distanceBetween(double[] a, double[] b) { | ||
| return Math.sqrt((a[0] - b[0]) * (a[0] - b[0]) + (a[1] - b[1]) * (a[1] - b[1])); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use haversine formula for geographic coordinate distance calculation
The distanceBetween method uses Euclidean distance which is inaccurate for geographic coordinates on the Earth's surface. Consider using the haversine formula for accurate results.
private static double distanceBetween(double[] a, double[] b) {
- return Math.sqrt((a[0] - b[0]) * (a[0] - b[0]) + (a[1] - b[1]) * (a[1] - b[1]));
+ // Haversine formula for distance between two points on the Earth's surface
+ double R = 6371000; // Earth radius in meters
+ double lat1 = Math.toRadians(a[0]);
+ double lat2 = Math.toRadians(b[0]);
+ double dLat = Math.toRadians(b[0] - a[0]);
+ double dLng = Math.toRadians(b[1] - a[1]);
+
+ double a1 = Math.sin(dLat/2) * Math.sin(dLat/2) +
+ Math.cos(lat1) * Math.cos(lat2) *
+ Math.sin(dLng/2) * Math.sin(dLng/2);
+ double c = 2 * Math.atan2(Math.sqrt(a1), Math.sqrt(1-a1));
+
+ return R * c;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static double distanceBetween(double[] a, double[] b) { | |
| return Math.sqrt((a[0] - b[0]) * (a[0] - b[0]) + (a[1] - b[1]) * (a[1] - b[1])); | |
| } | |
| private static double distanceBetween(double[] a, double[] b) { | |
| // Haversine formula for distance between two points on the Earth's surface | |
| double R = 6371000; // Earth radius in meters | |
| double lat1 = Math.toRadians(a[0]); | |
| double lat2 = Math.toRadians(b[0]); | |
| double dLat = Math.toRadians(b[0] - a[0]); | |
| double dLng = Math.toRadians(b[1] - a[1]); | |
| double a1 = Math.sin(dLat / 2) * Math.sin(dLat / 2) | |
| + Math.cos(lat1) * Math.cos(lat2) | |
| * Math.sin(dLng / 2) * Math.sin(dLng / 2); | |
| double c = 2 * Math.atan2(Math.sqrt(a1), Math.sqrt(1 - a1)); | |
| return R * c; | |
| } |
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/core/model/utils/PolygonUtils.java around lines 91
to 93, the distanceBetween method currently calculates Euclidean distance which
is inaccurate for geographic coordinates. Replace the calculation with the
haversine formula to compute the great-circle distance between two points on the
Earth's surface using their latitude and longitude values. This involves
converting degrees to radians, applying the haversine formula, and returning the
distance in meters or kilometers as appropriate.
| public static String getClosestPoint(List<Double> polygonPoints, double[] testPoint) { | ||
| int numVertices = polygonPoints.size() / 2; | ||
| double[] closestPoint = null; | ||
| double minDistance = Double.MAX_VALUE; | ||
|
|
||
| for (int i = 0; i < numVertices; i++) { | ||
| // Get the start and end points of the current edge | ||
| double startX = polygonPoints.get(2 * i); | ||
| double startY = polygonPoints.get(2 * i + 1); | ||
| double endX = polygonPoints.get(2 * ((i + 1) % numVertices)); | ||
| double endY = polygonPoints.get(2 * ((i + 1) % numVertices) + 1); | ||
|
|
||
| // Find the closest point on this edge | ||
| double[] candidatePoint = getClosestPointOnSegment( | ||
| startX, startY, endX, endY, testPoint[0], testPoint[1]); | ||
| double distance = distanceBetween(candidatePoint, testPoint); | ||
|
|
||
| // Update the closest point if necessary | ||
| if (distance < minDistance) { | ||
| minDistance = distance; | ||
| closestPoint = candidatePoint; | ||
| } | ||
| } | ||
|
|
||
| // Return the closest point as a space-separated string | ||
| return closestPoint[0] + " " + closestPoint[1]; | ||
| } |
There was a problem hiding this comment.
Fix potential null pointer exception and return value inconsistency
The getClosestPoint method:
- May return a null pointer exception if the polygon is empty
- Has JavaDoc that says it returns a result with distance and point, but actually returns just a string
/**
* Finds the minimum distance from a point to the polygon and the closest coordinate on the polygon.
*
* @param polygonPoints A list of doubles representing the polygon vertices
* (latitude and longitude pairs).
* @param testPoint A list of doubles representing the latitude and longitude of the test point.
- * @return A result containing the minimum distance and the closest point on the polygon.
+ * @return A space-separated string containing the latitude and longitude of the closest point on the polygon.
*/
public static String getClosestPoint(List<Double> polygonPoints, double[] testPoint) {
+ if (polygonPoints == null || polygonPoints.size() < 6 || testPoint == null || testPoint.length != 2) {
+ throw new IllegalArgumentException("Invalid polygon or test point");
+ }
+
int numVertices = polygonPoints.size() / 2;
double[] closestPoint = null;
double minDistance = Double.MAX_VALUE;
// Rest of the method...
// Return the closest point as a space-separated string
+ if (closestPoint == null) {
+ return ""; // Should never happen with validation, but as a safeguard
+ }
return closestPoint[0] + " " + closestPoint[1];
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static String getClosestPoint(List<Double> polygonPoints, double[] testPoint) { | |
| int numVertices = polygonPoints.size() / 2; | |
| double[] closestPoint = null; | |
| double minDistance = Double.MAX_VALUE; | |
| for (int i = 0; i < numVertices; i++) { | |
| // Get the start and end points of the current edge | |
| double startX = polygonPoints.get(2 * i); | |
| double startY = polygonPoints.get(2 * i + 1); | |
| double endX = polygonPoints.get(2 * ((i + 1) % numVertices)); | |
| double endY = polygonPoints.get(2 * ((i + 1) % numVertices) + 1); | |
| // Find the closest point on this edge | |
| double[] candidatePoint = getClosestPointOnSegment( | |
| startX, startY, endX, endY, testPoint[0], testPoint[1]); | |
| double distance = distanceBetween(candidatePoint, testPoint); | |
| // Update the closest point if necessary | |
| if (distance < minDistance) { | |
| minDistance = distance; | |
| closestPoint = candidatePoint; | |
| } | |
| } | |
| // Return the closest point as a space-separated string | |
| return closestPoint[0] + " " + closestPoint[1]; | |
| } | |
| /** | |
| * Finds the minimum distance from a point to the polygon and the closest coordinate on the polygon. | |
| * | |
| * @param polygonPoints A list of doubles representing the polygon vertices | |
| * (latitude and longitude pairs). | |
| * @param testPoint A list of doubles representing the latitude and longitude of the test point. | |
| * @return A space-separated string containing the latitude and longitude of the closest point on the polygon. | |
| */ | |
| public static String getClosestPoint(List<Double> polygonPoints, double[] testPoint) { | |
| if (polygonPoints == null || polygonPoints.size() < 6 || testPoint == null || testPoint.length != 2) { | |
| throw new IllegalArgumentException("Invalid polygon or test point"); | |
| } | |
| int numVertices = polygonPoints.size() / 2; | |
| double[] closestPoint = null; | |
| double minDistance = Double.MAX_VALUE; | |
| for (int i = 0; i < numVertices; i++) { | |
| // Get the start and end points of the current edge | |
| double startX = polygonPoints.get(2 * i); | |
| double startY = polygonPoints.get(2 * i + 1); | |
| double endX = polygonPoints.get(2 * ((i + 1) % numVertices)); | |
| double endY = polygonPoints.get(2 * ((i + 1) % numVertices) + 1); | |
| // Find the closest point on this edge | |
| double[] candidatePoint = getClosestPointOnSegment( | |
| startX, startY, endX, endY, testPoint[0], testPoint[1]); | |
| double distance = distanceBetween(candidatePoint, testPoint); | |
| // Update the closest point if necessary | |
| if (distance < minDistance) { | |
| minDistance = distance; | |
| closestPoint = candidatePoint; | |
| } | |
| } | |
| if (closestPoint == null) { | |
| return ""; // Should never happen with validation, but as a safeguard | |
| } | |
| // Return the closest point as a space-separated string | |
| return closestPoint[0] + " " + closestPoint[1]; | |
| } |
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/core/model/utils/PolygonUtils.java lines 44 to 70,
fix the potential null pointer exception by checking if the polygonPoints list
is empty before processing and returning an appropriate default or error value.
Also, update the method's return type or JavaDoc to ensure consistency: either
modify the method to return both distance and point as described or adjust the
JavaDoc to reflect that only the closest point as a string is returned.
| public static boolean isPointInsidePolygon(List<Double> polygonPoints, double[] testPoint) { | ||
| int intersectCount = 0; | ||
| int vertexCount = polygonPoints.size() / 2; | ||
|
|
||
| double testLat = testPoint[0]; | ||
| double testLng = testPoint[1]; | ||
|
|
||
| for (int i = 0; i < vertexCount; i++) { | ||
| double lat1 = polygonPoints.get(2 * i); | ||
| double lng1 = polygonPoints.get(2 * i + 1); | ||
| double lat2 = polygonPoints.get((2 * ((i + 1) % vertexCount))); | ||
| double lng2 = polygonPoints.get((2 * ((i + 1) % vertexCount)) + 1); | ||
|
|
||
| if (rayIntersectsEdge(testLat, testLng, lat1, lng1, lat2, lng2)) { | ||
| intersectCount++; | ||
| } | ||
| } | ||
|
|
||
| return (intersectCount % 2 == 1); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation to prevent potential exceptions
The method lacks validation for:
- The
polygonPointslist having an even number of elements - The
testPointarray having exactly 2 elements - The polygon having at least 3 vertices (required for a valid polygon)
These could lead to runtime exceptions or incorrect results.
public static boolean isPointInsidePolygon(List<Double> polygonPoints, double[] testPoint) {
+ if (polygonPoints == null || polygonPoints.size() < 6) { // At least 3 points needed for a polygon
+ return false; // Or throw IllegalArgumentException
+ }
+ if (polygonPoints.size() % 2 != 0) {
+ throw new IllegalArgumentException("Polygon points list must contain an even number of elements");
+ }
+ if (testPoint == null || testPoint.length != 2) {
+ throw new IllegalArgumentException("Test point must contain exactly 2 elements");
+ }
+
int intersectCount = 0;
int vertexCount = polygonPoints.size() / 2;
double testLat = testPoint[0];
double testLng = testPoint[1];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static boolean isPointInsidePolygon(List<Double> polygonPoints, double[] testPoint) { | |
| int intersectCount = 0; | |
| int vertexCount = polygonPoints.size() / 2; | |
| double testLat = testPoint[0]; | |
| double testLng = testPoint[1]; | |
| for (int i = 0; i < vertexCount; i++) { | |
| double lat1 = polygonPoints.get(2 * i); | |
| double lng1 = polygonPoints.get(2 * i + 1); | |
| double lat2 = polygonPoints.get((2 * ((i + 1) % vertexCount))); | |
| double lng2 = polygonPoints.get((2 * ((i + 1) % vertexCount)) + 1); | |
| if (rayIntersectsEdge(testLat, testLng, lat1, lng1, lat2, lng2)) { | |
| intersectCount++; | |
| } | |
| } | |
| return (intersectCount % 2 == 1); | |
| } | |
| public static boolean isPointInsidePolygon(List<Double> polygonPoints, double[] testPoint) { | |
| if (polygonPoints == null || polygonPoints.size() < 6) { // At least 3 points needed for a polygon | |
| return false; // Or throw IllegalArgumentException | |
| } | |
| if (polygonPoints.size() % 2 != 0) { | |
| throw new IllegalArgumentException("Polygon points list must contain an even number of elements"); | |
| } | |
| if (testPoint == null || testPoint.length != 2) { | |
| throw new IllegalArgumentException("Test point must contain exactly 2 elements"); | |
| } | |
| int intersectCount = 0; | |
| int vertexCount = polygonPoints.size() / 2; | |
| double testLat = testPoint[0]; | |
| double testLng = testPoint[1]; | |
| for (int i = 0; i < vertexCount; i++) { | |
| double lat1 = polygonPoints.get(2 * i); | |
| double lng1 = polygonPoints.get(2 * i + 1); | |
| double lat2 = polygonPoints.get((2 * ((i + 1) % vertexCount))); | |
| double lng2 = polygonPoints.get((2 * ((i + 1) % vertexCount)) + 1); | |
| if (rayIntersectsEdge(testLat, testLng, lat1, lng1, lat2, lng2)) { | |
| intersectCount++; | |
| } | |
| } | |
| return (intersectCount % 2 == 1); | |
| } |
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/core/model/utils/PolygonUtils.java around lines 15
to 34, add input validation at the start of the isPointInsidePolygon method to
ensure polygonPoints has an even number of elements, testPoint has exactly 2
elements, and polygonPoints represents at least 3 vertices (minimum 6 elements).
If any validation fails, throw an IllegalArgumentException with a clear message.
This prevents runtime exceptions and ensures the method operates on valid input.
| public class XPathPointInsidePolygon extends XPathFuncExpr{ | ||
| public static final String NAME = "inside-polygon"; | ||
| private static final int EXPECTED_ARG_COUNT = 2; | ||
| /** | ||
| * Returns true if the geopoint is inside the polygon, in meters, given objects to unpack. | ||
| * Ignores altitude and accuracy. | ||
| * Note that the arguments can be strings. | ||
| * Returns false if one of the arguments is null or the empty string. | ||
| */ | ||
| public XPathPointInsidePolygon() { | ||
| name = NAME; | ||
| expectedArgCount = EXPECTED_ARG_COUNT; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inaccurate function documentation
The JavaDoc comment is misleading and contains errors:
- It mentions "in meters" but the function returns a boolean, not a distance
- The method name
boundaryDistancedoesn't match its purpose
public class XPathPointInsidePolygon extends XPathFuncExpr{
public static final String NAME = "inside-polygon";
private static final int EXPECTED_ARG_COUNT = 2;
/**
- * Returns true if the geopoint is inside the polygon, in meters, given objects to unpack.
+ * Returns true if the geopoint is inside the polygon.
* Ignores altitude and accuracy.
* Note that the arguments can be strings.
* Returns false if one of the arguments is null or the empty string.
*/
public XPathPointInsidePolygon() {
name = NAME;
expectedArgCount = EXPECTED_ARG_COUNT;
}🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java around
lines 13 to 25, update the JavaDoc comment to accurately describe the function's
behavior by removing the incorrect mention of "in meters" since the function
returns a boolean indicating if a point is inside a polygon. Also, ensure that
any method names mentioned in the comment, such as `boundaryDistance`, are
corrected or removed to match the actual method purpose.
| public static boolean boundaryDistance(Object from, Object to) { | ||
| String unpackedFrom = (String)FunctionUtils.unpack(from); | ||
| String unpackedTo = (String)FunctionUtils.unpack(to); | ||
| if (unpackedFrom == null || "".equals(unpackedFrom) || unpackedTo == null || "".equals(unpackedTo)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename method to match its functionality
The method name boundaryDistance is misleading as it doesn't calculate or return a distance, but rather whether a point is inside a polygon.
- public static boolean boundaryDistance(Object from, Object to) {
+ public static boolean isPointInsidePolygon(Object polygonPoints, Object testPoint) {
- String unpackedFrom = (String)FunctionUtils.unpack(from);
- String unpackedTo = (String)FunctionUtils.unpack(to);
+ String unpackedPolygon = (String)FunctionUtils.unpack(polygonPoints);
+ String unpackedPoint = (String)FunctionUtils.unpack(testPoint);
- if (unpackedFrom == null || "".equals(unpackedFrom) || unpackedTo == null || "".equals(unpackedTo)) {
+ if (unpackedPolygon == null || "".equals(unpackedPolygon) || unpackedPoint == null || "".equals(unpackedPoint)) {And update the reference in evalBody:
@Override
protected Object evalBody(DataInstance model, EvaluationContext evalContext, Object[] evaluatedArgs) {
- return boundaryDistance(evaluatedArgs[0], evaluatedArgs[1]);
+ return isPointInsidePolygon(evaluatedArgs[0], evaluatedArgs[1]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public static boolean boundaryDistance(Object from, Object to) { | |
| String unpackedFrom = (String)FunctionUtils.unpack(from); | |
| String unpackedTo = (String)FunctionUtils.unpack(to); | |
| if (unpackedFrom == null || "".equals(unpackedFrom) || unpackedTo == null || "".equals(unpackedTo)) { | |
| return false; | |
| } | |
| public static boolean isPointInsidePolygon(Object polygonPoints, Object testPoint) { | |
| String unpackedPolygon = (String)FunctionUtils.unpack(polygonPoints); | |
| String unpackedPoint = (String)FunctionUtils.unpack(testPoint); | |
| if (unpackedPolygon == null || "".equals(unpackedPolygon) | |
| || unpackedPoint == null || "".equals(unpackedPoint)) { | |
| return false; | |
| } | |
| // ... rest of method unchanged ... | |
| } | |
| @Override | |
| protected Object evalBody(DataInstance model, EvaluationContext evalContext, Object[] evaluatedArgs) { | |
| return isPointInsidePolygon(evaluatedArgs[0], evaluatedArgs[1]); | |
| } |
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java around
lines 37 to 42, rename the method boundaryDistance to a name that reflects its
actual functionality, such as isPointInsidePolygon, since it returns a boolean
indicating point inclusion rather than a distance. Also, update all references
to this method, including the call in evalBody, to use the new method name to
maintain consistency.
| try { | ||
| String[] coordinates=unpackedFrom.split(" "); | ||
| List<Double> polygonList = new ArrayList<Double>(); | ||
|
|
||
| for (String coordinate : coordinates) { | ||
| polygonList.add(Double.parseDouble(coordinate)); | ||
| } | ||
| // Casting and uncasting seems strange but is consistent with the codebase | ||
| GeoPointData castedTo = new GeoPointData().cast(new UncastData(unpackedTo)); | ||
| boolean isInside= PolygonUtils.isPointInsidePolygon(polygonList,new double[]{castedTo.getLatitude(), castedTo.getLongitude()}); | ||
|
|
||
| return isInside; | ||
| } catch (NumberFormatException e) { | ||
| throw new XPathTypeMismatchException("point-in-boundary() function requires arguments containing " + | ||
| "numeric values only, but received arguments: " + unpackedFrom + " and " + unpackedTo); | ||
| } |
There was a problem hiding this comment.
Fix error message inconsistency and improve validation
The error message refers to point-in-boundary() function but the actual function name is inside-polygon. Also, validate that the polygon has an even number of coordinates.
try {
String[] coordinates=unpackedFrom.split(" ");
List<Double> polygonList = new ArrayList<Double>();
+ if (coordinates.length < 6) { // At least 3 points needed for a polygon
+ return false;
+ }
for (String coordinate : coordinates) {
polygonList.add(Double.parseDouble(coordinate));
}
+
+ // Ensure we have pairs of coordinates
+ if (polygonList.size() % 2 != 0) {
+ throw new XPathTypeMismatchException("inside-polygon() function requires an even number of coordinates for the polygon");
+ }
+
// Casting and uncasting seems strange but is consistent with the codebase
GeoPointData castedTo = new GeoPointData().cast(new UncastData(unpackedTo));
boolean isInside= PolygonUtils.isPointInsidePolygon(polygonList,new double[]{castedTo.getLatitude(), castedTo.getLongitude()});
return isInside;
} catch (NumberFormatException e) {
throw new XPathTypeMismatchException("point-in-boundary() function requires arguments containing " +
- "numeric values only, but received arguments: " + unpackedFrom + " and " + unpackedTo);
+ "inside-polygon() requires arguments containing numeric values only, but received arguments: " + unpackedFrom + " and " + unpackedTo);
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents (early access)
In src/main/java/org/javarosa/xpath/expr/XPathPointInsidePolygon.java between
lines 43 and 58, update the error message to correctly reference the function
name "inside-polygon" instead of "point-in-boundary()". Additionally, before
processing, add a validation step to check that the polygon coordinate list has
an even number of elements, throwing an appropriate exception if not, to ensure
the polygon coordinates are valid pairs.
|
This got merged by mistake but was eventually overeritten with #1455 , so luckily no harm done. |
Product Description
Old Pm_cluster for may beta release
Technical Summary
Safety Assurance
Safety story
Automated test coverage
QA Plan
Special deploy instructions
Rollback instructions
Review
Duplicate PR
Automatically duplicate this PR as defined in contributing.md.
Summary by CodeRabbit