current community

your communities

more stack exchange communities

company blog
Stack Exchange Inbox Reputation and Badges
tour help
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. Join them; it only takes a minute:

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top
up vote 4 down vote favorite

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);
    });

});
share|improve this question
up vote 2 down vote accepted
  • 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);
share|improve this answer
up vote 1 down vote

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.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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