1
\$\begingroup\$

Here is the part of working code:

  function SelectPoints(lat,lon){
var dist = document.getElementById("miles").value;

xy = [lat,lon];  //center point of circle

var theRadius = parseInt(dist) * 1609.34  //1609.34 meters in a mile 
//dist is a string so it's convered to an Interger.

selPts.length =0;  //Reset the array if selecting new points

  job.eachLayer(function (layer) {
    // Lat, long of current point as it loops through.
    layer_lat_long = layer.getLatLng();

    // Distance from our circle marker To current point in meters
    distance_from_centerPoint = layer_lat_long.distanceTo(xy);

    // See if meters is within radius, add the to array
    if (distance_from_centerPoint <= theRadius && $('#cf').is(":checked")) {
     selPts.push(layer.feature);  
}
})

    job2.eachLayer(function (layer) {
    // Lat, long of current point as it loops through.
    layer_lat_long = layer.getLatLng();

    // Distance from our circle marker To current point in meters
    distance_from_centerPoint = layer_lat_long.distanceTo(xy);

    // See if meters is within radius, add the to array
    if (distance_from_centerPoint <= theRadius && $('#vm').is(":checked")) {
     selPts.push(layer.feature);  
}
})

job3.eachLayer(function (layer) {
    // Lat, long of current point as it loops through.
    layer_lat_long = layer.getLatLng();

    // Distance from our circle marker To current point in meters
    distance_from_centerPoint = layer_lat_long.distanceTo(xy);

    // See if meters is within radius, add the to array
    if (distance_from_centerPoint <= theRadius && $('#bt').is(":checked")) {
     selPts.push(layer.feature);  
}
})

but it looks like it's going to be repeatable over next few layers. So far is only 3 layers job, job2 and job3, what doesn't look bad, but I am worried about the future.

Does anyone knows how to make this code a bit shorter?

Thanks & Regards,

\$\endgroup\$
2
  • 2
    \$\begingroup\$ I would rather return a new array of points instead of resetting a global variable. \$\endgroup\$
    – jakubiszon
    Nov 1, 2019 at 16:37
  • 1
    \$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers. Next time, please include all relevant code in the first revision. \$\endgroup\$
    – Mast
    Nov 1, 2019 at 16:53

1 Answer 1

2
\$\begingroup\$

A short review;

  • selPts.length = 0; //Reset the array if selecting new points always resets the array, even if no points will be added, also selPts = [] is more idiomatic
  • Seems like xy is not declared there, and it probably should be. Nor is selPts
  • I feel center describes more accurately the purpose of xy
  • Idiomatic JavaScript uses lowerCamelCase, so layer_lat_long -> layerLatLong etc.
  • In the end I went with layerLatLng since you get it from a function called getLatLng
  • I only knew selPts was selectedPoints due to comments, I think you should spell it out completely
  • You can extract the common logic into a function and pass that function to .eachLayer()

Obligatory rewrite:

function SelectPoints(latitude, longitude) {
  var dist = document.getElementById("miles").value,
    theRadius = parseInt(dist, 10) * 1609.34; //1609.34 meters in a mile

  center = [latitude, longitude]; //center point of circle
  selectedPoints.length = []; //Reset the array if selecting new points

  function updateSelectedPoints(layer){
    // Lat, long of current point as it loops through.
    layerLatLng = layer.getLatLng();
    // Distance from our circle marker To current point in meters
    distanceFromCenter = layerLatLng.distanceTo(center);
    // See if meters is within radius, add the to array
    if (distanceFromCenter <= theRadius){
      selectedPoints.push(layer.feature);
    }
  }

  if($('#cf').is(":checked")){
    job.eachLayer(updateSelectedPoints);
  }
  if($('#vm').is(":checked")){
    job2.eachLayer(updateSelectedPoints);
  }
  if($('#bt').is(":checked")){
    job3.eachLayer(updateSelectedPoints);
  }
}
\$\endgroup\$
3
  • \$\begingroup\$ Your solution doesn't work. I believe, that it's my fault, because I haven't attached a full code. Now my query has been edited. Could you please look on it again? Thank you very much. \$\endgroup\$
    – Geographos
    Nov 1, 2019 at 16:35
  • \$\begingroup\$ The point would not be to copy paste the approach (I clearly was not able to test it, you can't trust that code), but to give you an idea on how to solve the issue. Are you saying that you can not extract the common functionality in a common function? \$\endgroup\$
    – konijn
    Nov 1, 2019 at 16:39
  • \$\begingroup\$ @MariuszKrukar, how it does not work? Add more context and description of where it fails \$\endgroup\$ Nov 1, 2019 at 17:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.