3
\$\begingroup\$

I've been working on a crime map demo - and I am keen to learn more about how to rewrite this application to use redux. What is the best approach, why - what is wrong with the way its currently written - too reliant on jQuery?

What would be the advantage of using redux - to keep component data available for other sister components - master parent components?

//index.js

import React from 'react';
import ReactDOM from 'react-dom';
import './index.css';

var $ = require("jquery");
import MapChart from './modules/mapChart/MapChart';

var CrimeMap = React.createClass({

    getInitialState: function() {
      return {
        result: ''
      };
    },

    componentDidMount: function () {
        this.serverRequest = $.get(this.props.source, function (result) {          
          this.setState({
            result: result
          });
        }.bind(this));
    },

    componentWillUnmount: function() {
      this.serverRequest.abort();
    },

    getLayers: function(data){
      var items = [];

      items.push(  <MapChart
                      key="1"  
                      data= {data} /> );

      return items;      
    },

    render: function () {
       var result = this.state.result;
       return (
            <div className="apps">
                {this.getLayers(result)}
            </div>
        );
    }
});

//https://data.police.uk/docs/method/crime-street/
ReactDOM.render(
    <CrimeMap source="https://data.police.uk/api/crimes-street/all-crime?poly=52.268,0.543:52.794,0.238:52.130,0.478&date=2013-01" />,
    document.getElementById('root')
);

//MapChart.js

import React, { Component } from 'react';
import ReactDOM from 'react-dom';
var $ = require("jquery");
import './MapChart.css';

import {Map, InfoWindow, Marker, GoogleApiWrapper} from 'google-maps-react';

export class MapContainer extends Component {

    render() {
        function getIcon(category){
            //anti-social-behaviour
            //burglary
            //criminal-damage-arson
            //drugs
            //other-theft
            //shoplifting
            //vehicle-crime
            //other-crime

            switch(category) {
                case "anti-social-behaviour":
                    return 'http://maps.google.com/mapfiles/ms/icons/green-dot.png'
                    break;
                case "burglary":
                    return 'http://maps.google.com/mapfiles/ms/icons/red-dot.png'
                    break;
                case "criminal-damage-arson":
                    return 'http://maps.google.com/mapfiles/ms/icons/yellow-dot.png'
                    break;
                case "drugs":
                    return 'http://maps.google.com/mapfiles/ms/icons/purple-dot.png'
                    break;
                case "other-theft":
                    return 'http://maps.google.com/mapfiles/ms/icons/pink-dot.png'
                    break;
                case "shoplifting":
                    return 'http://maps.google.com/mapfiles/ms/icons/blue-dot.png'
                    break;
                case "other-crime":
                    return 'http://maps.google.com/mapfiles/ms/icons/orange-dot.png'
                    break;
                default:
                    return 'http://maps.google.com/mapfiles/ms/icons/ltblue-dot.png'
            }
        }

        const data = [];

        $.each(this.props.data, function( index, value ) {
            var obj = {
                "label": value.category,
                "lat": value.location.latitude,
                "lng": value.location.longitude,
                "icon": getIcon(value.category)
            }
            data.push(obj);
        });

        return (
          <Map 
            google={this.props.google} 
            initialCenter={{
              lat: 52.225827,
              lng: 0.484861
            }}
            zoom={11}
          >

            {
              data.map(function (item, index) {
                return (
                  <Marker
                    key={index}
                    icon={item.icon}
                    title={item.label}
                    name={item.label}
                    position={{lat: item.lat, lng: item.lng}} />
                )
              })
            }

          </Map>
        );
    }
}

export default GoogleApiWrapper({
  apiKey: 'xx'
})(MapContainer)

//MapChart.css

 /* Always set the map height explicitly to define the size of the div
   * element that contains the map. */
  #map {
    height: 100%;
  }
  /* Optional: Makes the sample page fill the window. */
  html, body {
    height: 100%;
    margin: 0;
    padding: 0;
  }
\$\endgroup\$
  • 1
    \$\begingroup\$ How do I go about adding in redux or improving this - I lost out on a job because of this code \$\endgroup\$ – The Old County Aug 4 '17 at 13:02
1
\$\begingroup\$

An app this small has no need for a state container like redux. From what you've shown there are no interactions, and you don't really have any state that changes or needs to be managed/synced across components. The location state appears to be fetched and rendered once. Redux would add complexity without any real benefit.

As far as improvements to the code go, there are a number of things that could be addressed. They're in comments in the code snippets below, but I'll recap at the end as well.

index.js

import React, { Component } from 'react'
import ReactDOM from 'react-dom'
import MapChart from './modules/mapChart/MapChart'
import './index.css'

// this is how you'll get your icon links
// instead of a switch with loads of repetitive bytes
const iconMap = {
  'anti-social-behaviour':  'green-dot',
  'burglary':               'red-dot',
  'criminal-damage-arson':  'yellow-dot',
  'drugs':                  'purple-dot',
  'other-theft':            'pink-dot',
  'shoplifting':            'blue-dot',
  'vehicle-crime':          'orange-dot',
  'other-crime':            'ltblue-dot'
}

// this is a class because it needs state
class CrimeMap extends Component {
  // to do this you have to make sure you have
  // the transform-class-properties feature in babel
  // otherwise just set your initial state in a constructor
  // constructor(props) {
  //   super(props)
  //   this.state = { markers: [] }
  // }
  state = {
    markers: []
  }

  componentDidMount() {
    // use fetch instead of jQuery
    // jQuery is a big lib to be loading for some fetching
    fetch(this.props.source)
      .then( response => response.json() )
      .then(
        json => this.setState({
          markers: this.mapLayerData(json)
        }),
        err => { // handle errors }
      )
  }

  // store only the data you want to pass as props to each Marker
  // instead of mapping it directly in MapChart every time it renders
  mapLayerData(markers) {
    // use a standard map function instead of $.each
    // destructuring saves time and repetition
    return markers.map(({ category, location }) => ({
      // use a template string and a simple map of icon names to get your icon uri
      icon: `http://maps.google.com/mapfiles/ms/icons/${iconMap[category]}.png`,
      label: category,
      name: category,
      position: {
        lat: location.latitude,
        lng: location.longitude
      }
    }))
  }

  render() {
    return (
      <div className="app">
        // there's only one layer, so render it directly
        <MapChart markers={this.state.markers} />
      </div>
    )
  }
}

ReactDOM.render(
  <CrimeMap source="https://data.police.uk/api/crimes-street/all-crime?poly=52.268,0.543:52.794,0.238:52.130,0.478&date=2013-01" />,
  document.getElementById('root')
)

MapChart.js

import React from 'react'
// only load what you're actually using
import { Map, Marker, GoogleApiWrapper } from 'google-maps-react'
import './MapChart.css'

// this is a stateless component, it doesn't need state or event handlers
const MapContainer = ({ google, markers }) => (
  <Map 
    google={google} 
    initialCenter={{
      lat: 52.225827,
      lng: 0.484861
    }}
    zoom={11}
  >
    {
      markers.map((marker, i) =>
        // since you've mapped your data to just the props you need
        // you can just spread it into the component
        <Marker key={i} {...marker} />
      )
    }
  </Map>
)

export default GoogleApiWrapper({
  apiKey: 'xx'
})(MapContainer)

You don't have to use destructuring, spread, arrow functions or template strings is you don't want, but you're already using babel to compile so I'd recommend it.

The main takeaways:

  • you don't need jQuery
  • get familiar with common array methods like map, filter and reduce
  • avoid repetitive things (eg the icon uris)
  • make your data useful before passing it around as props
  • passing flat props is nicer than objects as props, and pretty clutch when it comes to optimizing performance (eg props.name, props.label vs props.data.name, props.data.label)
  • use a stateless component when you don't need state or internal methods (eg lifecycle hooks and event handlers)

As far as learning redux goes, you'll really need a better example app to make much sense of it. You need to at least have some interactions that affect your state. Dan Abramov (creator of Redux) has two excellent video series introducing it via a simple counter app. There's an intro series here and a second series here, both free and thorough. I'd recommend going through those to start.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ No this example needs to have redux added to it. Even though its a small application. They all want to see redux on this \$\endgroup\$ – The Old County Aug 8 '17 at 9:30
  • \$\begingroup\$ I need the "redux" added to this example - its what employees would want to see \$\endgroup\$ – The Old County Aug 10 '17 at 10:58
0
\$\begingroup\$

At a high level your on the right track as to why redux is often used. Your answer of: to keep component data available for other sister components - master parent components?, is right on the edge as to why it's useful.

For a simple one liner I would say that redux provides a single source of truth and easily reasoned about logic for your whole application's state. For waaay more details than what would fit here, I recommend reading the Three Principles

After reading your comment, it seems you did this for an interview. Well the interviewer probably was wanting to hear keywords such as, "testability, simplicity, single source of truth", etc, in regards to redux.

Now for the jQuery aspect, I would say jQuery is unwarranted in this situation. The reason being, that data fetching and iterating over data, the two situations you used jQuery for, could easily have been done with native javascript. e.g Using fetch(url), and data.forEach.

Replacing $.get

fetch(this.props.source)
  .then(function(response) { return response.json(); })
  .then(result => {this.setState({result:result})

Replacing your usage of $.each

const data = this.props.data.map(value => {
  return {
    label: value.category,
    lat: value.location.latitude,
    lng: value.location.longitude,
    icon: getIcon(value.category)
  };
});

Going back to your comment of this being in the context of an interview, leveraging jQuery in your solution was probably a sign to the interviewer that your understanding of native javascript could be improved.

|improve this answer|||||
\$\endgroup\$
  • \$\begingroup\$ This was a "do at home" test - but in response to the answer looking for an actual code solution along with some of the explanations you just gave \$\endgroup\$ – The Old County Aug 6 '17 at 7:48
  • \$\begingroup\$ "understanding of native javascript" - well I was more wondering if there was a react version of looping through the array - \$\endgroup\$ – The Old County Aug 6 '17 at 7:56
  • \$\begingroup\$ @TheOldCounty, sure, see the examples showing how to map from jQuery to native javascript. \$\endgroup\$ – enjoylife Aug 7 '17 at 3:11
  • \$\begingroup\$ Thank but still need to flesh this out "refactors this code to use redux and any other improvements" \$\endgroup\$ – The Old County Aug 7 '17 at 8:37
  • \$\begingroup\$ I need the "redux" added to this example - its what employees would want to see \$\endgroup\$ – The Old County Aug 10 '17 at 10:58

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.