Province from coordinate #361
Conversation
|
|
|
Dear @mediv0 |
| && (point.longitude < ((polygon[j][0] - polygon[i][0]) * (point.latitude - polygon[i][1]) / (polygon[j][1] - polygon[i][1]) + polygon[i][0]))) { | ||
| odd = !odd; | ||
| } | ||
| j = i; |
There was a problem hiding this comment.
please move the j = i inside the for loop
for (let i = 0, j = polygon.length - 1; i < polygon.length; j = i++) { ... }| const provinces = GeoJSONData; | ||
|
|
||
| function pointInPolygon(polygon: number[][], point: Point): boolean { | ||
| let odd = false; |
There was a problem hiding this comment.
the variable odd doesn't make any sense here ( naming )
please change it to something more meaningful
like intersect isIntersecting
| if (((polygon[i][1] > point.latitude) !== (polygon[j][1] > point.latitude)) | ||
| && (point.longitude < ((polygon[j][0] - polygon[i][0]) * (point.latitude - polygon[i][1]) / (polygon[j][1] - polygon[i][1]) + polygon[i][0]))) { |
There was a problem hiding this comment.
using the Ray casting algorithm is good idea here but this if statement is so cluttered and hard to read
you could define some local variables to hold values instead of using them directly like polygon[i][1]
const xi = polygon[i][0];
const yi = polygon[i][1];
const xj = polygon[j][0];
const yj = polygon[j][1];
const { longitude: x, latitude: y } = point
// so much simpler to read
((yi > y) !== (yj > y)) && (x < (xj - xi) * (y - yi) / (yj - yi) + xi);| let foundProvince: GeoJSONFeature | undefined; | ||
| for (let index = 0; index < provinces.features.length; index++) { | ||
| const province = provinces.features[index]; | ||
| const isInsideProvince = pointInPolygon(province.geometry.coordinates[0][0], pointToCheck); |
There was a problem hiding this comment.
make another variable here
const coordinates = province.geometry.coordinates[0][0];
| const normalizedProvinceObject: Province = { | ||
| properties: { | ||
| 'name:fa': foundProvince.properties.name || "", | ||
| 'name:en': foundProvince.properties['name:en'] || "", | ||
| }, | ||
| geometry: foundProvince.geometry |
There was a problem hiding this comment.
why are you having keys like name:fa ?
it would not be clean to destruct this object and get the result
i would suggest change to something like
const normalizedProvinceObject: Province = {
name: {
fa: foundProvince.properties.name || "",
en: foundProvince.properties['name:en'] || "",
},
geometry: foundProvince.geometry
}
// accessing
// .1
const {fa, en} = province.name
// .2
const { name } = province
name.fa // تهران
name.en // tehran
| interface Province { | ||
| properties: { | ||
| 'name:fa': string; | ||
| 'name:en': string; | ||
| }; | ||
| geometry: object; | ||
| } |
There was a problem hiding this comment.
- what is the point of showing users geometry of the given coordinates
- as the functions indicates, you will provide a set of coordinate and get a result respectively
it would be better to remove this property entirely and focus on the main purpose of this function which is finding a province
something like:
const { fa, en } = findProvinceFromCoordinate(...)- it should have a type anything other than raw object
|
Thank you for your comments @mediv0 , |
That's awesome I'll check tomorrow |
Lot's of Eradaat 😉✌️ |
|
@ali-master what do you think ? |
Added findProvinceFromCoordinate module - tests passed and readme update