Codereview

[thomaswilburn]

Margie,

Here's the pull request with my code changes. I'll leave notes in a few specific places, but I thought I'd give an overview here. I do want to note, you don't have to actually accept this pull, it's just to illustrate a few things. Most of the changes I've made can still be back-ported to your code, although you'll have to work around my structural changes.

The first thing you'll notice is that I've moved a bunch of stuff into separate JS files or modules, and added a build step that uses Browserify. This basically lets us write Node-style require()-based code in smaller sets, instead of working from one huge file. I feel like that makes it easier to understand. The final file is not committed, so if you'd like to see it, you'll need to install Grunt and the package modules. I saved them to the package.json file, so you should just be able to run an npm install to get them all. You'll note that I'm using this to load jQuery, LoDash, and Modernizr instead of using separate script tags.

After that, I pulled the API calls out and moved them to their own module, as well as the templates. This cleaned up a little bit of repetition in the code, and it also let me add a caching layer. Second requests to OpenStates won't need to hit the network, so they'll be super-fast. It's possible that the browser was already caching these, but a separate module means that we could add pre-processing there, if we wanted.

Finally, I spent some time re-working the loops and event flow for the page. It should be a little clearer now, and faster as well. For example, it's better to loop over the committees once and do the filtering by hand, rather than doing it through LoDash/Underscore and needing to loop through four or five times.

If I were going to keep working on this, I think one of the things I'd want to do is move it to a minimal MVC or Angular. You have a lot of "state" in this application, which is currently not really being tracked. Instead, stuff just gets put into the page, and then it reacts in a fairly ad-hoc way. It would be good to have more of a separation between parts of the code that process data, and parts that do actual rendering and output.

Hope this helps, feel free to comment if you have any questions.

[thomaswilburn]

These styles should almost certainly be moved into a class.