6
\$\begingroup\$

Please provide general and any specific criticism of the following which is intended to provide a map of SF transit cars. The code can be viewed in the working version and also in a group of gists.

If this isn't the appropriate place or way to request a review please let me know. Feedback received so far indicates the solution is amateurish and I would like to better understand the better/preferred solutions--specifically what and where to focus. Overall this was a first-pass at a solution intended to reflect a productive day's work.

import './the-controls.js';

class TheMap extends HTMLElement{
    constructor(){
        super();
        this.attachShadow({mode:'open'}).innerHTML = `
<style>
:host{display:block;width:100%;height:100%;}

svg{border:1px dotted #555;background:#0df;height:100%;width:100%;position:fixed;}
g[src*='neighborhoods']{fill:green;stroke:#ccff0033;stroke-width:4px;}
g[src*='arteries']{display:none;fill:none;stroke:#000;stroke-width:0.5;}
g[src*='freeways']{display:none;fill:none;stroke:#fff;stroke-opacity:0.7;stroke-width:0.5;}
g[src*='streets']{display:none;stroke:#fff;fill:none;stroke-opacity:0.8;stroke-width:0.2px;}
.show-freeways g[src*='freeways'],
.show-streets g[src*='streets'],
.show-arteries g[src*='arteries']{display:inline;}
.vehicles circle{fill:red;fill-opacity:0.7;}
.vehicles circle:hover{fill:#cf0;fill-opacity:1;stroke:#555;stroke-width:0.1;}
.vehicles text{display:none;fill:var(--dark);font-size:1rem;pointer-events:none;}
.vehicles g:active text,
.vehicles g:hover text{display:block;}
button{font-weight:bold;}
button.active{background-color:var(--dark);color:white;}
/*.vehicles g{transition:all 500ms;}*/
</style>
<svg>
    <g class=layers></g>
    <g class=overlay>
<foreignobject class="filter" x="2.5rem" y="2rem" width="150" height="100%">
<the-controls></the-controls>
</foreignobject>
    </g>
</svg>
        `;
        this._setup = false;
        this.map = d3.select(this.shadowRoot.querySelector('svg'))
        this.layers = d3.select(this.shadowRoot.querySelector('.layers'))
        this.controls = this.shadowRoot.querySelector('the-controls')
        this.data = {
            time: 0
//'http://webservices.nextbus.com/service/publicJSONFeed?command=vehicleLocations&a=sf-muni&t=0'
            ,tags: {}
            ,taglist: []
            ,vehicles: []
            ,timer: 0
            ,interval: 30
            ,scale: 100000
            // San Francisco
            ,lon: -122.4, lat: 37.76
            ,projection: null
            ,pathprojector: null
            // 
            ,content: './sfmaps/neighborhoods.json ./sfmaps/arteries.json ./sfmaps/freeways.json ./sfmaps/streets.json'
        }
        this.addEventListener('change-active', this.vehicleDrawer)
    }
    tagisactive(active, tag){
        if(tag.active) active[ tag.name ] = tag;
        return active;
    }
    vehicleisactive(item){
        return this[ item.routeTag ] ? true : false;
    }
    vehicleDrawer(){
        // show the active-tag vehicles
        var data = this.data, $, vehicleList = data.vehicles.filter(this.vehicleisactive,
                data.taglist.reduce(this.tagisactive, {}) );

        requestAnimationFrame(()=>{
            // update
            $ = this.layers
            .select('g.vehicles')
            .selectAll("g")
            .data(vehicleList)
            .each(this.vehicleDetails)
            ;
            // enter
            $.enter()
            .append("g")
            .each(this.vehicleDetails)
            ;
            // exit
            $.exit().remove();
        })
    }
    //item = {dirTag:"7____O_F00", heading:"225", id:"6719", lat:"37.784794", lon:"-122.403969", predictable:"true", routeTag:"7", secsSinceReport:"17", speedKmHr:"19"}
    vehicleDetails(item, i, vehicleList){
        var $;
        this.setAttribute('transform', `translate(${item.position[0]}, ${item.position[1]}) scale(0.5)`);
        $ = this.querySelector('text');
        if($){
            $.textContent = item.routeTag;
        }else{
            this.innerHTML = `<circle cx=0 cy=0 r=5 /><text>${ item.routeTag }</text>`;
        };
    }
    vehiclePostion(item){
        item.position = this.projection([+item.lon, +item.lat]);
    }
    tagSorter(a,b){
        return a.name < b.name ? -1 :(a.name > b.name ? 1 : 0); 
    }
    updateFinish(res){
        this.data.timer = setTimeout(()=>{ this.update(); }, 1000 * this.data.interval);
        console.log('updated',res);
        return res;
    }
    update(){
        clearTimeout(this.data.timer);
        d3.json(`http://webservices.nextbus.com/service/publicJSONFeed?command=vehicleLocations&a=sf-muni&t=${this.data.time}`)
        .then((json)=>{
            var taglist, data = this.data;
            // get all the unique routeTags as a sorted list
            taglist = json.vehicle.reduce(this.vehiclehash, data).taglist;
            data.taglist = taglist = Array.from(new Set(taglist)).sort(this.tagSorter);
            data.vehicles = json.vehicle;
            // easier to interpret the position here than passing the pieces and parts around
            data.vehicles.forEach(this.vehiclePostion, data)

            this.vehicleDrawer();
            this.controls.model = data;

            return this.updateFinish(json);
        }).catch((res)=>{
            return this.updateFinish(json);
        });

    }
    vehiclehash(data, item){
        var name = item.routeTag, tag = data.tags[ name ];
        if(!tag){
            tag = data.tags[ name ] = {name:name, active:true};
        };
        data.taglist.push( tag );
        item.tag = tag;
        return data;
    }
    layerDrawer(json){
        requestAnimationFrame(()=>{
            var data = this.data, $;
            $ = this.layers
            .select(`g[src="${json.path}"]`)
            .selectAll("path")
            .data(json.features)
            .attr("d", data.pathprojector)  
            ;
            $.enter()
            .append("path")
            .attr("d", data.pathprojector)  
            ;
            $.exit().remove();
        });
    }
    // zooming adjusts transform and visible layers/detail
    mapView(position={}){
        cancelAnimationFrame(this._rafmap);
        this._rafmap = requestAnimationFrame(()=>{
            var z = position.k, view, layer, layers = this.layers;
            // I don't know d3
            view = layers._groups[0][0].classList;
            layer = 'show-streets';
            if(view.contains(layer)){
                if(z <= 6) view.remove(layer);
            }else if(z > 6){
                view.add(layer);
            };
            layer = 'show-arteries';
            if(view.contains(layer)){
                if(z <= 3) view.remove(layer);
            }else if(z > 3){
                view.add(layer);
            };
            layer = 'show-freeways';
            if(view.contains(layer)){
                if(z <= 1.5) view.remove(layer);
            }else if(z > 1.5){
                view.add(layer);
            };
            layers.attr("transform", position);
        });

    }
    connectedCallback(){
        var data;
        if(!this._setup){
        // initial setup: projection, load and draw layers, start updating
            data =  this.data;
            data.projection = d3.geoMercator().scale(data.scale).center([+data.lon, +data.lat]);
            data.pathprojector = d3.geoPath().projection(data.projection);

            this.map.call(
                d3.zoom()
                .scaleExtent([1,30])
                .on('zoom', ()=>{ this.mapView(d3.event.transform) })
            )

            data.content.trim().split(/\s+/).map((path, i) => {
                this.layers.append('g').attr('src', path)
                d3.json(path)
                .then((json)=>{
                    json.path = path;
                    this.layerDrawer(json);
                    return json;
                })
                .catch((err)=>{
                    console.warn('problem layer',path,err);
                    return err;
                })
                return path;
            });
            this.layers.append('g').attr('class','vehicles');
        }
        this.update();
    }
    disconnectedCallback(){
        // stop updating
        clearTimeout(this.data.timer);
    }
}
window.customElements.define('the-map', TheMap);


class TheControls extends HTMLElement{
    constructor(){
        super();
        this.attachShadow({mode:'open'});
        // {tags: {N: {}...}, taglist: []}
        this.data = {};
        this.addEventListener('click', this.clicked);
    }
    clicked(e){
        var $ = e.composedPath()[0], tags = this.data.tags, prev;
        e.stopPropagation();
        switch($.nodeName){
        case 'INPUT':
            switch($.value){
            case 'all':
                Array.from(this.shadowRoot.querySelectorAll('button')).forEach(($)=>{
                    tags[$.value].active = true;
                    $.classList.add('active');
                });
            break;
            case 'clear':
                Array.from(this.shadowRoot.querySelectorAll('button')).forEach(($)=>{
                    tags[$.value].active = false;
                    $.classList.remove('active');
                });
            break;
            };
            this._previous = '';
            this.dispatchEvent(new CustomEvent('change-active', {detail: this.data, cancelable: true, composed: true, bubbles: true}));
        break;
        case 'BUTTON':
            if(e.metaKey || e.ctrlKey){
                prev = this.shadowRoot.querySelector(`button[value="${this._previous}"]`);
                if(prev && prev !== $){
                    tags[prev.value].active = prev.classList.toggle('active');
                }
            };
            this._previous = $.value;
            tags[$.value].active = $.classList.toggle('active');
            this.dispatchEvent(new CustomEvent('change-active', {detail: this.data, cancelable: true, composed: true, bubbles: true}));
        break;
        };
    }
    //<button value="10" class="active">10</button>
    tagHTML(item){
        return `<button value="${ item.name }" class="${ item.active ? 'active':'' }">${ item.name }</button>`
    }
    render(){
        this.shadowRoot.innerHTML = `
<style>
button{font-weight:bold;}
button.active{background-color:var(--dark);color:white;}
</style>
<input type=button value=all>
<input type=button value=clear>
<br>
${ this.data.taglist.map(this.tagHTML).join(' ') }
        `;
    }
    get model(){
        return this.data;
    }
    set model(data){
        this.data = data;
        this.render();
        return true;
    }
}
window.customElements.define('the-controls', TheControls);
\$\endgroup\$
  • 1
    \$\begingroup\$ Welcome to Code Review: this is indeed the appropriate SE place to ask your question. Well, I'm pretty much the only D3 answerer around here these days, so here is my 2 cents: your code per se is not amateurish (despite the fact that it has in fact some unnecessary bits and things that could be simplified), if they said that to your face it was pretty rude. However, let's forget the code for a moment and focus on the final result, which is the datavis itself: it's not exactly beautiful the way it is right now. Maybe they're talking about the datavis itself, not the code... (continue) \$\endgroup\$ – Gerardo Furtado Jun 29 '18 at 5:41
  • \$\begingroup\$ ... and, if that's the case, they may be right: the design is not good, if you allow me to say. That being said, there are a lot of things that should be changed in the vis, from the color palette to the zoom behaviour. However, changing these is not only outside the scope of Code Review but also it will take so much time/energy that I'm afraid no one will help you regarding the redesign. So, my advice is: think about the best design for this vis, study the options, create the new design and, then, you can come back asking for a code review again. Best of luck to you. \$\endgroup\$ – Gerardo Furtado Jun 29 '18 at 5:43
  • \$\begingroup\$ the feedback was specific to the code (not the visual design) and is quoted here in full: Though i like the fact the the candidate used web components (mostly for the newness rather than for web component itself), the challenge is poorly designed: everything is split among two components; no further level of abstraction/structure. These 2 components contain lots of junior patterns (this[ item.routeTag ] ? true : false;), coding style could have been better, and some code repetition that could have been abstracted. \$\endgroup\$ – jimmont Jun 29 '18 at 7:22
  • \$\begingroup\$ Well, that's hard... I don't use shadow DOM or things like that, so let's see if someone else can write a review for your code. \$\endgroup\$ – Gerardo Furtado Jun 29 '18 at 8:57
4
\$\begingroup\$

Just as Gerardo hinted at in comments, I admit that I am not well-versed with d3 and at most can offer general JavaScript feedback. Nonetheless here is what I came up with:

  1. Since you are using ES-6 features like modules, classes, etc. consider using let and const where appropriate. Also, instead of Array.from() the spread syntax can be used.

    For instance, in TheControls::clicked() there are lines like this:

    Array.from(this.shadowRoot.querySelectorAll('button')).forEach(($)=>{
    

    Which could be simplified using spread syntax:

    [...this.shadowRoot.querySelectorAll('button')].forEach(($)=>{
    

    And a related optimization may be to store those button elements in a variable when the DOM is ready instead of querying each time the method is run.

  2. The redundant code in TheMap::MapView() can be simplified. Specifically the following lines:

    layer = 'show-streets';
    if(view.contains(layer)){
        if(z <= 6) view.remove(layer);
    }else if(z > 6){
        view.add(layer);
    };
    layer = 'show-arteries';
    if(view.contains(layer)){
        if(z <= 3) view.remove(layer);
    }else if(z > 3){
        view.add(layer);
    };
    layer = 'show-freeways';
    if(view.contains(layer)){
        if(z <= 1.5) view.remove(layer);
    }else if(z > 1.5){
        view.add(layer);
    };
    

    One technique would be to use a plain old JSON object to map the layer names to the threshold values:

    const layerThresholds = {
        'show-streets': 6,
        'show-arteries': 3,
        'show-freeways': 1.5
    };
    Object.keys(layerThresholds).forEach(layer => {
        const threshold = layerThresholds[layer];
        if (view.contains(layer)) {
            if (z <= threshold) {
                 view.remove(layer);
            }
        }
        else if (z > threshold) {
            view.add(layer);
        }
    });
    

    One could aim for shorter code using various techniques like a ternary operator:

    Object.keys(layerThresholds).forEach(layer => {
        const threshold = layerThresholds[layer];
        view[z <= threshold ? 'remove' : 'add'](layer);
    });
    
  3. The list of JSON files to load layers from, i.e.

    ,content: './sfmaps/neighborhoods.json ./sfmaps/arteries.json ./sfmaps/freeways.json ./sfmaps/streets.json'
    

    is a string. Is there a reason not to use an array, like this?

    ,content: ['./sfmaps/neighborhoods.json', './sfmaps/arteries.json', './sfmaps/freeways.json', './sfmaps/streets.json']
    

    Obviously it requires a few extra keystrokes, but then when it is used, there is no need to trim and split the string - just iterate over the filenames.

  4. One minor shortcut is for arrow functions - when only one parameter is needed, then the parentheses can be omitted. For example:

    .then((json)=>{
    

    Can be simplified to:

    .then(json=>{
    
| improve this answer | |
\$\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.