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
  • 1
    \$\begingroup\$ How do I go about adding in redux or improving this - I lost out on a job because of this code \$\endgroup\$ Commented Aug 4, 2017 at 13:02

2 Answers 2

Reset to default
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.

\$\endgroup\$
2
  • \$\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\$ Commented Aug 8, 2017 at 9:30
  • \$\begingroup\$ I need the "redux" added to this example - its what employees would want to see \$\endgroup\$ Commented Aug 10, 2017 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.

\$\endgroup\$
5
  • \$\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\$ Commented Aug 6, 2017 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\$ Commented Aug 6, 2017 at 7:56
  • \$\begingroup\$ @TheOldCounty, sure, see the examples showing how to map from jQuery to native javascript. \$\endgroup\$ Commented Aug 7, 2017 at 3:11
  • \$\begingroup\$ Thank but still need to flesh this out "refactors this code to use redux and any other improvements" \$\endgroup\$ Commented Aug 7, 2017 at 8:37
  • \$\begingroup\$ I need the "redux" added to this example - its what employees would want to see \$\endgroup\$ Commented Aug 10, 2017 at 10:58

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.