4
\$\begingroup\$

I wrote this code for an personal application, just to help me visualize some data related to driving. I have one JSON on my machine, this JSON contains a set of locations (lat, lngs) and every location has a category (integer number).

After I fetch this file I want to use Google's Road API, to snap the points on the road. Unfortunately it only allows 100 points per request. And depending on the "track" I can have usually 400+ points.

At this point, this application is only maintained by me. But I used this as an excuse to re-learn Javascript (including the new stuff only new browsers have).

I used Promises and I was used to the callback hell of before and after reading so much good stuff about Promises I got really frustrated seeing this huge chaining I made.

It's working, can't complain about that. But, looks messy.

document.addEventListener('DOMContentLoaded', function(evt){
    'use strict';

    const apiKey = '-----------';

    let rawMap = new google.maps.Map(document.getElementById('rawMap'), {
        center: {lat: 0, lng: 0},
        zoom: 10
    });

    let snappedMap = new google.maps.Map(document.getElementById('snappedMap'), {
        center: {lat: 0, lng: 0},
        zoom: 10
    });

    const colors = [
        '#FF0000',
        '#00FF00',
        '#0000FF',
        '#FFFF00',
        '#00FFFF',
        '#FF00FF',
        '#000000',
    ];

    fetch('output.json').then(response => response.json()).then(function(points){
        for (let i = 0 ; i < points.length - 1 ; ++i) {
            let path = new google.maps.Polyline({
                path: [points[i], points[i+1]],
                strokeColor: colors[points[i]['cat']],
                strokeWeight: 5
            });

            path.setMap(rawMap);
        }

        let bounds = new google.maps.LatLngBounds();
        points.forEach(point => bounds.extend(point));
        rawMap.fitBounds(bounds)
        snappedMap.fitBounds(bounds);

        let requests = [];

        for (let i = 0; i < points.length ; i += 100) {
            let params = new URLSearchParams();
            params.append('interpolate', 'true');
            params.append('key', apiKey)
            params.append('path', points.slice(i, i + 100).map(point => `${point['lat']},${point['lng']}`).join('|'));

            requests.push(fetch(`https://roads.googleapis.com/v1/snapToRoads?${params}`));
        }

        return Promise.all(requests)
                        .then(responses => Promise.all(responses.map(response => response.json())))
                        .then(function(jsons){
                            for (let i = 0, lastIndex = -1; i < jsons.length ; ++i) {
                                let snappedPoints = jsons[i].snappedPoints;

                                for (let snappedPoint of snappedPoints) {
                                    if ('originalIndex' in snappedPoint) {
                                        lastIndex = i * 100 + snappedPoint.originalIndex;
                                    }

                                    snappedPoint['cat'] = points[lastIndex]['cat'];
                                }
                            }

                            return jsons;
                        });
    }).then(function(jsons) {
        return jsons.reduce((ret, json) => ret.concat(json.snappedPoints), [])
    }).then(function(snappedPoints){
        for (let i = 0; i < snappedPoints.length - 1; i++) {
            let path = new google.maps.Polyline({
                path: [
                    {
                        lat: snappedPoints[i].location.latitude,
                        lng: snappedPoints[i].location.longitude
                    },
                    {
                        lat: snappedPoints[i+1].location.latitude,
                        lng: snappedPoints[i+1].location.longitude
                    }
                ],
                strokeWeight:5,
                strokeColor: colors[snappedPoints[i]['cat']]
            });

            path.setMap(snappedMap);
        }
    }).catch(function(error){
        console.log(error);
    });

});
\$\endgroup\$
2
\$\begingroup\$
  • Divide processing in simple functions, then chain them in linear fashion.
  • Use literal .property or ['property'] consistently, don't mix them as it's the same in JS
  • Concatenate arrays in one operation via Array.prototype.concat.apply([], arrayOfArrays) instead of reduce that creates intermediate arrays (it's slow and memory-hogging). However, if the number of elements may exceed 32k (the max number of arguments in some browsers) it's safer to concat in chunks of 10k by using a loop and .slice.

// your init part

let points;

const getJSON = (response) => response.json();

const drawPoints = (json) => {
    // will be used later for snapping to roads
    points = json;

    for (let i = 0; i < points.length - 1; ++i) {
        let path = new google.maps.Polyline({
            path: [points[i], points[i + 1]],
            strokeColor: colors[points[i].cat],
            strokeWeight: 5
        });
        path.setMap(rawMap);
    }

    // fit map bounds to the points
    let bounds = new google.maps.LatLngBounds();
    points.forEach(point => bounds.extend(point));
    rawMap.fitBounds(bounds);
    snappedMap.fitBounds(bounds);
};

const requestSnapToRoads = () => {
    let requests = [];
    for (let i = 0; i < points.length; i += 100) {
        let params = new URLSearchParams();
        params.append('interpolate', 'true');
        params.append('key', apiKey)
        params.append('path', points
            .slice(i, i + 100)
            .map(point => `${point.lat},${point.lng}`)
            .join('|'));
        requests.push(fetch(`https://roads.googleapis.com/v1/snapToRoads?${params}`));
    }
    return Promise.all(requests).then(responses => Promise.all(responses.map(getJSON)));
};

const applySnapToRoads = (jsons) => {
    let lastIndex = -1;
    let allSnappedPoints = jsons.map((json, i) => {
        for (let snappedPoint of json.snappedPoints) {
            if ('originalIndex' in snappedPoint) {
                lastIndex = i * 100 + snappedPoint.originalIndex;
            }
            snappedPoint.cat = points[lastIndex].cat;
        }
        return json.snappedPoints;
    });
    return [].concat.apply([], allSnappedPoints);
};

const drawSnappedPoints = (snappedPoints) => {
    const getPointAt = (index) => ({
        lat: snappedPoints[index].location.latitude,
        lng: snappedPoints[index].location.longitude
    });
    for (let i = 0; i < snappedPoints.length - 1; i++) {
        let path = new google.maps.Polyline({
            path: [getPointAt(i), getPointAt(i + 1)],
            strokeWeight: 5,
            strokeColor: colors[snappedPoints[i].cat]
        });
        path.setMap(snappedMap);
    }
};

// chain them!

fetch('output.json')
    .then(getJSON)
    .then(drawPoints)
    .then(requestSnapToRoads)
    .then(applySnapToRoads)
    .then(drawSnappedPoints)
    .catch(console.log);
\$\endgroup\$
  • \$\begingroup\$ Sorry for the delayed comment. But without further ado: WOW! This was like really impressive, I really liked the result and been studying it a lot. Couple of questions though, just to understand the rationale and standards, 1) why creating functions with the arrow functions? just to preserve the this object? 2) Why one function per step even though it would never (until now) be used again, for readability? 3) I thought one of the plus of Promises was the fact it doesn't need to declare variables out from its scope, so that let points; would never exist or am I really wrong? \$\endgroup\$ – Antonio Ribeiro Nov 4 '16 at 2:07
  • \$\begingroup\$ 1. Personally, I'd use local function declarations as I'm used to ES5 but here arrow functions are used for syntax consistency only 2. When a function does just one simple task it's much easier to understand for anyone (this is explained better in many coding guides) 3. Regarding let points, I've used the simplest solution (you use out-of-scope variables anyway). I could have transferred the variable via Promise chain for better encapsulation. \$\endgroup\$ – wOxxOm Nov 4 '16 at 2:23
1
\$\begingroup\$

I would use promises only where it's necessary, a couple of the transformations here can just be put into a regular variable without problems, the .json() calls for example.

  • Extract duplicate code like the map creation since all the parameters are the same except for the element ID.
  • Extract semantically useful code into functions.
  • Be consistent with the semicolons.
  • Maybe also use functional mapping to iterate over arrays to get of the manual for-loops.
  • The first nested return Promise.all(requests).then(...) should probably be put one layer up so it flows better, that is, ... return Promise.all(requests); }).then(function(responses){...}).

Otherwise yeah, looks good.

\$\endgroup\$

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.