2
\$\begingroup\$

I have this code and would like to have it reviewed:

function thisClient() {
  "use strict"
  var self = this

  var width = 960,
    height = 500,
    centered, data

    this.init = function () {
      //now.receiveLocation = function(message) {
      //    console.log(message)
      //    // FIXME only push markers depending on the country/adm1 level
      //    self.drawMarker(message)
      //}
      self.drawMap()
    }

  this.fileExists = function (url) {
    "use strict"
    var http = new XMLHttpRequest()
    http.open('HEAD', url, false)
    http.send()
    return http.status != 404
  }

  this.quantize = function (d) {
    "use strict"
    return "q" + Math.floor(getCountyNorm(d.id) * 10) + "-9";
  }

  // Map code
  this.drawMap = function () {
    "use strict"
    var map = d3.geo.equirectangular().scale(150)
    self.path = d3.geo.path().projection(map)

    self.svg = d3.select("#map").append("svg")
      .attr("width", "100%")
      .attr("height", "90%")
      .attr("viewBox", "0 0 " + width + "  " + height)
      .attr("preserveAspectRatio", "xMidYMid")

    // Add a transparent rect so that zoomMap works if user clicks on SVG
    self.svg.append("rect")
      .attr("class", "background")
      .attr("width", width)
      .attr("height", height)
      .on("click", self.zoomMap)

    // Add g element to load country paths
    self.g = self.svg.append("g")
      .attr("id", "countries")
    // Load topojson data
    d3.json("./topo/world.json", function (topology) {
      self.g.selectAll("path")
      //.data(topology.features)
      .data(topojson.object(topology, topology.objects.countries).geometries)
        .enter().append("path")
        .attr("d", self.path)
        .attr("id", function (d) {
        return d.properties.name
      })
      //.attr("class", data ? self.quantize : null)
      .on("mouseover", function (d) {
        d3.select(this)
          .style("fill", "orange")
          .append("svg:title")
          .text(d.properties.name)
        //self.activateTooltip(d.properties.name)
      })
        .on("mouseout", function (d) {
        d3.select(this)
          .style("fill", "#aaa")
        //self.deactivateTooltip()
      })
        .on("click", self.zoomMap)
      // Remove Antarctica
      self.g.select("#Antarctica").remove()
    })
  } // end drawMap

  this.zoomMap = function (d) {
    "use strict"
    var x, y, k

    if (d && centered !== d) {
      var centroid = self.path.centroid(d)
      x = centroid[0]
      y = centroid[1]
      k = 2
      centered = d
      self.loadCountry(d, x, y, k)
    } else {
      // zoom out, this happens when user clicks on the same country
      x = width / 2
      y = height / 2
      k = 1
      self.centered = null
      // as we zoom out we want to remove the country layer
      self.svg.selectAll("#country").remove()
    }

    self.g.selectAll("path")
      .classed("active", centered && function (d) {
      return d === centered
    })

    self.g.transition()
      .duration(1000)
      .delay(100)
      .attr("transform", "translate(" + width / 2 + "," + height / 2 + ")scale(" + k + ")translate(" + -x + "," + -y + ")")
      .style("stroke-width", 1.5 / k + "px")

  } // end zoom function

  this.loadCountry = function (d, x, y, k) {
    "use strict"
    // we remove the country
    self.svg.selectAll("#country").remove()
    // load country json file
    var adm1_key = d.id + "_adm1"
    var adm1_path = "./topo/" + d.id + "_adm1.json"

    // check to see if country file exists
    if (!self.fileExists(adm1_path)) {
      console.log("We couldn't find that country!")
    } else {
      console.log("Load country overlay")
      var country = self.svg.append("g").attr("id", "country")

      self.countryGroup = self.svg.select("#country")
      d3.json(adm1_path, function (error, topology) {
        var regions = topology.objects
        for (var adm1_key in regions) {
          var o = regions[adm1_key]
        }
        self.countryGroup.selectAll("path")
          .data(topojson.object(topology, o).geometries)
          .enter().append("path")
          .attr("d", self.path)
          .attr("transform", "translate(" + width / 2 + "," + height / 2 + ")scale(" + k + ")translate(" + -x + "," + -y + ")")
          .attr("id", function (d) {
          return d.properties.NAME_1
        })
          .classed("country", true)
          .attr("class", "country")
          .style("stroke-width", 1.5 / k + "px")
          .on("mouseover", function (d) {
          d3.select(this)
            .style("fill", "#6C0")
            .append("svg:title")
            .text(d.properties.NAME_1)
        })
          .on("mouseout", function (d) {
          d3.select(this)
            .style("fill", "#000000")
        })
          .on("click", function (d) {
          console.log('clicked on country')
          self.loadProjects()
        })
      })
    } // end else
  }

  this.loadProjects = function () {
    console.log('loadProjects')

  } // end loadProjects

  this.init()
} // end thisClient

jQuery(function () {
  thisClient = new thisClient()
})
\$\endgroup\$
3
\$\begingroup\$

From a once over:

  • Because you have use strict on top in function thisClient() {, you do not need it within functions declared within thisClient like this.fileExists
  • I am all for Spartan coding but variable like d and k have to go
  • Using the http method HEAD is cool, I learned something
  • Don't keep commented out code
  • You do not use semicolons, please use semicolons; what will the neighbors say?
  • The lack of semicolons also breaks JsHint
  • Your aligning is a bit odd in this part:

    d3.json("./topo/world.json", function (topology) {
      self.g.selectAll("path")
      //.data(topology.features)
      .data(topojson.object(topology, topology.objects.countries).geometries)
        .enter().append("path")
        .attr("d", self.path)
        .attr("id", function (d) {
        return d.properties.name
      })
      //.attr("class", data ? self.quantize : null)
      .on("mouseover", function (d) {
        d3.select(this)
          .style("fill", "orange")
          .append("svg:title")
          .text(d.properties.name)
        //self.activateTooltip(d.properties.name)
      })
        .on("mouseout", function (d) {
        d3.select(this)
          .style("fill", "#aaa")
        //self.deactivateTooltip()
      })
        .on("click", self.zoomMap)
    

    I would put the .on next tothe }):

    d3.json("./topo/world.json", function (topology) {
      self.g.selectAll("path")
      .data(topojson.object(topology, topology.objects.countries).geometries)
        .enter().append("path")
        .attr("d", self.path)
        .attr("id", function (d) {
        return d.properties.name
      }).on("mouseover", function (d) {
        d3.select(this)
          .style("fill", "orange")
          .append("svg:title")
          .text(d.properties.name)
        //self.activateTooltip(d.properties.name)
      }).on("mouseout", function (d) {
        d3.select(this)
          .style("fill", "#aaa")
        //self.deactivateTooltip()
      }).on("click", self.zoomMap)
    
  • It is kind of evil to create a constructor and then replace it with an instance of it self, you cant possibly name that variable correctly ;)
  • Please do not use console.log in production code
  • You should assign color constants .style("fill", "#6C0") to properly named variables, somewhere on the top
  • There must be better ways than this to get the last adm1_key:

    for (var adm1_key in regions) {
      var o = regions[adm1_key]
    }
    
\$\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.