2
\$\begingroup\$

I have a script that makes a database call and then filters the results. After filtering it pushes it to an array and shows it on a map.

Here is my function:

function getDriving() {
    pointArray.length = 0;
    markers.length = 0;
    var todayval = new Date();


    var url = $('#map').data('request-url2');
    $.getJSON(url,
        function (data) {
            $.each(data,
                function (i, item) {

                    markers.push({
                        'location': new google.maps.LatLng(item.Latitude, item.Longitude),
                        'map': map,
                        'weight': item.Speed,
                        'radius': 10,
                        'date': item.CurrentDateTime,
                        'imei': item.Imei
                    });
                });
            imei = parseInt($('#selectVehicles').val());

            var startValue = $('#startDate').val().split("/");
            var endValue = $('#endDate').val().split("/");
            var valDateStart = new Date(startValue[2], startValue[1] - 1, startValue[0]);
            var valDateEnd = new Date(endValue[2], endValue[1] - 1, endValue[0]);

            filtered = markers.filter(function (marker) {
                var getDate = marker.date.match(/\d/g).join('');
                var markerDate = new Date(parseFloat(getDate));
                return ((markerDate >= valDateStart && markerDate <= valDateEnd) && marker.imei === imei);
            });
            var pointArray = new google.maps.MVCArray(filtered);

            heatmap = new google.maps.visualization.HeatmapLayer({
                data: pointArray
            });
            heatmap.setMap(map);
        });
};

It works well, but I want to know if everything is okay with the code or how I can improve it.

\$\endgroup\$
2
\$\begingroup\$

Scope of Variables

Unless imei is used outside of the code in your post (which would be grounds for making it off-topic), then you should declare it as local to the function using var (or let if you want it to have block scope. The same is true for filtered and heatmap. That way those variables won't be global, which could cause confusion if you did use var to declare a variable inside a function but not before that.

For more information on that topic, see the section Keep your scopes close and your scope even closer on this page.

Accessing DOM elements multiple times via $()

DOM lookups are slow. It is wise to cache them once in a variable and then refer to the variable when needed. This is also mentioned in the aforementioned article under the section Cache DOM Lookups

Those lookups could be added above the function (possibly assigned in a DOM ready function like jQuery's $(function(){}), though it is argued that isn't necessary with modern browsers):

var map = $('#map');
var selectedVehicles = $('#selectVehicles');
var startDate = $('#startDate');
var endDate = $('#endDate');

Then those variables can be used later on when the function is run- for example:

var url = $('#map').data('request-url2');

Can be updated to use map:

var url = map.data('request-url2');

And map could also be used when setting up the Google Map instance, since presumably that is the same element used for the map.

Using parseInt() without a radix

If you are going to use parseInt(), it is wise to specify the radix using the second parameter - unless you are using a unique number system like hexidecimal, octal, etc. then specify 10 for decimal numbers.

Always specify this parameter to eliminate reader confusion and to guarantee predictable behavior. Different implementations produce different results when a radix is not specified, usually defaulting the value to 10.1

So I would update the line:

imei = parseInt($('#selectVehicles').val());

To pass the radix 10:

imei = parseInt($('#selectVehicles').val(), 10);

That way, if a value like 022 is entered, it isn't interpreted as the octal value (I.e. Decimal number 18).


1https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/parseInt#Parameters

\$\endgroup\$
1
\$\begingroup\$

There are a lot of things that can be improved, especially when working with inputs, then use keyup or blur so you don't have to do a lot of computation when getDriving() is called.

I'll focus on $.each(): this method is extremely slow and can be solved with native JavaScript methods like Array.reduce().

function (data) {
     imei = parseInt($('#selectVehicles').val());

     var startValue = $('#startDate').val().split("/");
     var endValue = $('#endDate').val().split("/");
     var valDateStart = new Date(startValue[2], startValue[1] - 1, startValue[0]);
     var valDateEnd = new Date(endValue[2], endValue[1] - 1, endValue[0]);

    filtered = data.reduce(function(arr, item){
       var getDate = item.CurrentDateTime.match(/\d/g).join('');
       var itemDate = new Date(parseFloat(getDate));

       if((itemDate >= valDateStart && itemDate <= valDateEnd) && item.Imei === imei){
          arr.push({
             'location': new google.maps.LatLng(item.Latitude, item.Longitude),
             'map': map,
             'weight': item.Speed,
             'radius': 10,
             'date': item.CurrentDateTime,
             'imei': item.Imei
         });
      }
      return arr;
    }, []);

    var pointArray = new google.maps.MVCArray(filtered);

    heatmap = new google.maps.visualization.HeatmapLayer({
       data: pointArray
    });
    heatmap.setMap(map);
}

I ended up removing the filter as well; when you get the data, you can just reduce the array into the actual output you want and then just check if it satisfies the condition and if not then do not add it to the list.

Also, I noticed that variables imei, startValue, endValue, varDateStart and valDateEnd never change value so just put them above the array.reduce().

\$\endgroup\$
  • \$\begingroup\$ imei and other values can be changed \$\endgroup\$ – Balance Jan 26 '18 at 14:13
  • \$\begingroup\$ You have a very short time frame to change it while the reduce function is executing, so you could potentially disable them meanwhile. Your filter function also suffers from this side effect, the code is working the same way. \$\endgroup\$ – Pavlo Jan 26 '18 at 14:17

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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