Created
October 29, 2013 22:46
-
-
Save rclark/7224042 to your computer and use it in GitHub Desktop.
little code review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
function onEachFeature(feature, layer) { | |
/* From the perspective of reading this function, it would be easier if you think about it in a linear fashion | |
What does this function do? | |
1) get a single feature | |
2) parse some specific properties out of the feature | |
3) use those properties to generate a string of HTML | |
4) build a popup from that HTML | |
Right now its a little hard to read because you're jumping around between those steps a bit | |
*/ | |
// Step #2 Gather properties from the feature: | |
// One `var`, many variables, comma-separated | |
var pdf = feature.properties.Pdf.split(','), // keep the data simple, make the app "more complex" | |
label = feature.properties.Label, | |
bbox = layer.getBounds().toBBoxString(), | |
/* | |
This whole function can be boiled down to pretty much a single `.map` function, where you take the last result | |
But aside from that, there are some things to think about in how you wrote this one: | |
var preview = function() { | |
this.path; << Why `this`? If it doesn't do anything for you, don't use it. | |
_.each(pdf, function(item){ | |
this.path = 'assets/' + item + '.png'; | |
}); | |
return this.path; | |
} | |
*/ | |
preview = _.chain(pdf) // see http://underscorejs.org/#chain | |
.map(function (item) { return 'assets/' + item + '.png'; }) // get an array of URLs | |
.last() // take the last one | |
.value(), // return the "chain's value": http://underscorejs.org/#value | |
/* This is another one that boils down to an array.map function. Always use array.map if your | |
goal is to take one array, process each item in it, and end up with another array | |
var download_path = function(item) { | |
base_uri = document.baseURI.toString(); <<< I don't think you need to make absolute URLs here | |
whole_uri = base_uri.substring(0, base_uri.lastIndexOf("/") + 1); | |
return whole_uri + item + '.pdf' | |
} | |
var links = function() { | |
this.html; << the use of `this` here makes things really confusing | |
var template = _.template('<ul><a href= <%= path %>> <%= name %></a></ul>'); <<< this is so simple that you don't really need templating helper | |
_.each(pdf, function(item) { | |
path = download_path(item); | |
if (pdf.length > 1) { | |
this.html += template({path: path, name: item}); | |
} else { | |
this.html = template({path: path, name: item}); | |
} | |
}); | |
return this.html; | |
} | |
*/ | |
_.each([], function () {}) | |
[].forEach(funtion () {}) | |
[].map(function () {}) | |
links = _.map(pdf, function (item) { | |
var path = item + '.pdf'; | |
return '<li><a href="' + path + '">' + item + '</a></li>'; | |
}).join(''); | |
/* There's no need for a dedicated function here, because you're only going to call it once. | |
var template = function(title, preview, links, bbox) { | |
return '<div class=title><h4>' + title + ' Study Area</h4></div>' + | |
'<div class=content>' + | |
'<div class=preview><img src=' + preview + '></div>' + | |
'<div class=links><h6>Downloadable Maps:</h6>' + | |
'<div class=download>' + links + '</div>' + | |
'<div id=zoom>' + | |
'<button type="button" onclick="doZoom(' + bbox + ')" class="btn btn-success">Zoom to this study area</button>' + | |
'</div>' + | |
'</div>' + | |
'</div>'; | |
} | |
*/ | |
// Step #3 Turning the feature into a string of html | |
// I'm starting a new `var` here just for clarity. This one is a complex bit of content | |
var html = '<div class=title><h4>' + label + ' Study Area</h4></div>'; | |
html += '<div class=content>'; | |
html += '<div class=preview><img src=' + preview + '></div>'; | |
html += '<div class=links><h6>Downloadable Maps:</h6>'; | |
html += '<div class=download><ul>' + links + '</ul></div>'; | |
html += '<div id=zoom>'; | |
html += '<button type="button" onclick="doZoom(' + bbox + ')" class="btn btn-success">Zoom to this study area</button>'; | |
html += '</div></div></div>'; | |
// Step #4 Create a popup and bind it to the layer | |
var popup = L.popup({ | |
'maxWidth': 500, | |
'minWidth': 250 | |
}).setContent(html); | |
layer.bindPopup(popup); | |
} | |
var doZoom = function (bbox0, bbox1, bbox2, bbox3) { | |
//var sw = new L.LatLng(bbox1, bbox0); | |
//var ne = new L.LatLng(bbox3, bbox2); | |
//var bounds = new L.LatLngBounds(sw, ne); | |
// Little shortcut here | |
var bounds = L.latLngBounds([[bbox1, bbox0], [bbox3, bbox2]]); | |
app.map.fitBounds(bounds); | |
} | |
// I understand the effeciency here, but it takes another step to understand what's happening = harder to read | |
// When there are so few layers, I'd say just add them | |
/* | |
for (var key in app.layers) { | |
app.layers[key].addTo(app.map); | |
} | |
*/ | |
app.layers.baseLayer.addTo(app.map); | |
app.layers.fissuresLayer.addTo(app.map); | |
app.layers.studyAreas.addTo(app.map); | |
L.geocoderControl().addTo(app.map); | |
d3.json('data/studyareas.json', function (err, content) { | |
}); |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment