From 31a4a39c8eca3bdd6776daaa0f3d60556e7944e2 Mon Sep 17 00:00:00 2001
From: Aaron Krause <aaroncrows@Aarons-MacBook-Pro.local>
Date: Thu, 19 Jan 2017 10:54:49 -0800
Subject: [PATCH] submission comments

---
 scripts/controllers/installationsController.js |  3 ++-
 scripts/models/artMap.js                       |  2 ++
 scripts/models/findMap.js                      |  3 ++-
 scripts/models/installations.js                | 17 +++++++++++++++--
 scripts/views/searchView.js                    | 10 +++++++++-
 5 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/scripts/controllers/installationsController.js b/scripts/controllers/installationsController.js
index 638dd55..2d9b022 100644
--- a/scripts/controllers/installationsController.js
+++ b/scripts/controllers/installationsController.js
@@ -7,7 +7,8 @@
   installationsController.index = function(ctx, next) {
     installationView.index(ctx.installation);
   };
-
+  // this is a great use of middleware. Using it to compose async functionality is
+  // one of the things it's best for.
   installationsController.loadByMedium = function(ctx, next) {
     var mediumData = function(installationsInMedium) {
       ctx.installation = installationsInMedium;
diff --git a/scripts/models/artMap.js b/scripts/models/artMap.js
index 118d95c..f35ed21 100644
--- a/scripts/models/artMap.js
+++ b/scripts/models/artMap.js
@@ -17,6 +17,8 @@ function showPosition(position) {
 }
 
 artMap.initMap = function () { //creates map
+  // it may have been helpful to clear up what these numbers represent.
+  // maybe with something like: var pdxCoords = {lat: 45.5315, lng: -122.6668}
   var lat = 45.5315;
   var lng = -122.6668
   var mapOptions = {
diff --git a/scripts/models/findMap.js b/scripts/models/findMap.js
index 4152121..bbfbebc 100644
--- a/scripts/models/findMap.js
+++ b/scripts/models/findMap.js
@@ -45,7 +45,7 @@ findMap.setPinCoords =  function (filteredArray){
 }
 
 
-
+ //prefer camelCase for function and variable names
  findMap.place_all_Pins =  function(locationData){ //takes a 2d array of coords
     var opts = {};
     if (allPins.length) {
@@ -64,6 +64,7 @@ findMap.setPinCoords =  function (filteredArray){
       google.maps.event.addListener(marker, "click", function (event) {
         var latitude = this.position.lat();
         var obj;
+        // Nice use of both routing and passing functions
         Installation.findWhere('lat', latitude,  function(data) {
           page('/loveart/' + data[0].id);
         });
diff --git a/scripts/models/installations.js b/scripts/models/installations.js
index afd0bb6..84ab252 100644
--- a/scripts/models/installations.js
+++ b/scripts/models/installations.js
@@ -6,7 +6,13 @@
       this[e] = opts[e];
     },this);
   }
-
+  // The technical term for a function like this is a noop (no operation)
+  // it's possible that you may need something like this. Usually, though,
+  // you'd use a noop for a function that was expecting to be passed a callback
+  // and would throw a TypeError because it was trying to call something that
+  // isn't there. Looking through your code it seems like none of the places where
+  // you pass myTemp actually have to have a callback so you likely could have just
+  // left it out.
   function myTemp(){
   };
 
@@ -78,7 +84,14 @@
     }); //execute select
   };//fetchAll
 
-
+  // the following functions (the ones that start with 'all') likely could have
+  // been refactored to take another argument. Like:
+  // Installation.allBy = function(category, callback) {
+  //    webDB.execute('SELECT DISTINCT ' + category + ' FROM Installations', callback);
+  // }
+  // or if you wanted to get even fancier about it you could go through a list of all of
+  // the categories and return the results of each query as an array. This gets tough though
+  // because you have to manage the sequence of async calls returning.
   Installation.allMediums = function(callback) {
     webDB.execute('SELECT DISTINCT medium FROM Installations', callback);
   };
diff --git a/scripts/views/searchView.js b/scripts/views/searchView.js
index d79f62d..7eb77e8 100644
--- a/scripts/views/searchView.js
+++ b/scripts/views/searchView.js
@@ -6,7 +6,11 @@
 
   searchView.populateFilters = function(){
     var template = Handlebars.compile($('#option-template').text());
-
+    //The if statements here don't appear to be doing anything. Since
+    //you put your append logic inside the conditional it's going to
+    //evaluate it either way. Which means that it's running the query
+    //and appending the results. No matter what the results thereof
+    //are.
     Installation.allMediums(function(rows) {
       if ($('#medium-filter').append(rows.map(function(row){
         return template({val: row.medium});
@@ -37,6 +41,10 @@
 
   searchView.handleFilters = function () {
     $('#filters').on('change', 'select', function() {
+      // this is a neat way to solve the problem of getting Which
+      // filter was interacted with from the element itself. It might
+      // be a bit more idiomatic and stable to store the needed
+      // information as a data attribute instead of on the id.
       var f = this.id.replace('-filter', '');
       var v = $(this).val();
       Installation.findWhere(f, v, function (filteredArray) {