Review notes

source/js/application.js

@@ -1,20 +1,24 @@
+ColorApp = {}; // This is the top-level file, define your namespace here
+
 $('document').ready(function(){
-  var selectors = {
+  var controller, binder,
+    selectors = {
     makeGridAction: "button.new",
     clearGridAction: "button.clear",
     saveGridAction: "submit",
   };
 
-  ColorApp.view = new ColorApp.View( {
-    gridSelector: ".default-grid ul",
-    tileSelector: "li",
+  controller = new ColorApp.Controller({
+    view: new ColorApp.View({
+      gridSelector: ".default-grid ul",
+      tileSelector: "li",
+    })
   });
 
-  ColorApp.controller = new ColorApp.Controller({view: ColorApp.view});
-  ColorApp.controller.loadDefaultGrid();
+  controller.loadDefaultGrid();
 
-  ColorApp.binder = new ColorApp.Binder(selectors, ColorApp.controller);
-  ColorApp.binder.bind();
+  binder = new ColorApp.Binder(selectors, controller);
+  binder.bind();
 })
 
 

source/js/src/binder.js

@@ -1,43 +1,83 @@
+/* I generally don't like a class like this.  I feel like
+ * the controller should have a method called init() or
+ * somethign like it that knows how to set up the events
+ * based on a config object (like you have in application.js)
+ *
+ * This Binder thing is a pattern I use with Phase 2's to break
+ * up the pattern of jQuery soup.  It's not terrible, but it's
+ * a bit unconventional
+ *
+ *
+ * It seems like there should be something like
+ *
+  controller = new ColorApp.Controller({
+    view: new ColorApp.View({
+      gridSelector: ".default-grid ul",
+      tileSelector: "li",
+    })
+  });
+
+  controller.loadDefaultGrid();
+
+  binder = new ColorApp.Binder(selectors, controller);
+
+
+ That moves to...
+
+  new ColorApp.Controller({
+    view: new ColorApp.View({
+      gridSelector: ".default-grid ul",
+      tileSelector: "li",
+    })
+  }).init();
+
+ WHERE init() does the work of...
+
+  this.loadDefaultGrid();
+
+ and
+
+  binder = new ColorApp.Binder(selectors, controller);
+ *
+ */
+
 ColorApp.Binder = function(targets, controller){
   this.targets = targets;
   this.controller = controller;
 }
 
 ColorApp.Binder.prototype = {
   bind: function(){
-          this.bindClearGridAction();
-          this.bindColorTileAction();
-          this.bindSaveGridAction();
+    this.bindClearGridAction();
+    this.bindColorTileAction();
+    this.bindSaveGridAction();
   },
 
   bindClearGridAction: function(){
-                          var controller = this.controller,
-                          but = this.targets.clearGridAction;
+    var controller = this.controller,
+    but = this.targets.clearGridAction;
 
-                          $(but).on('click', function(){
-                            controller.loadDefaultGrid();
-                          })
+    $(but).on('click', function(){
+      controller.loadDefaultGrid();
+    })
   },
 
   bindColorTileAction: function(){
-                          var controller = this.controller,
-                          but = this.targets.tileSelector;
+    var controller = this.controller,
+    but = this.targets.tileSelector;
 
-                          $(".default-grid ul li").on('click', function(e){
-                            controller.colorTile(e);
-                          })
+    $(".default-grid ul li").on('click', function(e){
+      controller.colorTile(e);
+    })
   },
 
   bindSaveGridAction: function(){
-                          var controller = this.controller,
-                          but = this.targets.saveGridAction;
-
-                          $("form").submit(function(e) {
-                            controller.saveGrid(e);
-                            e.preventDefault();
-                          })
-  },
-
-
+    var controller = this.controller,
+    but = this.targets.saveGridAction;
 
+    $("form").submit(function(e) {
+      controller.saveGrid(e);
+      e.preventDefault();
+    })
+  }
 }

source/js/src/controller.js

@@ -5,57 +5,67 @@ ColorApp.Controller = function(config){
 
 ColorApp.Controller.prototype = {
   loadDefaultGrid: function() {
-              this.grid = new Grid();
-              this.populateGrid(this.grid);
-              this.view.displayDefaultGrid(this.grid);
+    this.grid = new Grid();
+    this.populateGrid(this.grid);
+    this.view.displayDefaultGrid(this.grid);
+
+    /* The above line is a classic give away of not understanding how OO should
+     * work.
+     *
+     * Why is the controller telling the View what to do to draw the screen?
+     * Why can't the controller throw itself to the view and then the *view*
+     * can decide what it needs from the thing it receives (i.e. .grid) to
+     * draw?  Here you're making the contorller know how to display.  Don't.
+     * Consult "Inversion of Dependency"
+     */
   },
 
   populateGrid: function(grid) {
-                  for (var i = 0; i < grid.size; i++){
-                    grid.addTile();
-                  }
+    for (var i = 0; i < grid.size; i++){
+      grid.addTile();
+    }
   },
 
   colorTile: function(e) {
-                var index = e.target.getAttribute('class');
-                var gridSelector = "." + e.target.parentElement.parentElement.getAttribute('class') + " ul";
-                var tile = this.grid.tiles[index]
-                var newColor = this.randColor();
-                tile.setColor(newColor);
-                this.view.setColor(tile, index, gridSelector);
+    var index = e.target.getAttribute('class');
+    /* Gross! */
+    var gridSelector = "." + e.target.parentElement.parentElement.getAttribute('class') + " ul";
+    var tile = this.grid.tiles[index]
+    var newColor = this.randColor();
+    tile.setColor(newColor);
+    this.view.setColor(tile, index, gridSelector);
   },
 
   randColor: function(){
-                return "#" + Math.floor(Math.random()*16777215).toString(16);
+    /* See code smell called "MAGIC NUMBER" What do these
+     * numbers mean?
+     */
+    return "#" + Math.floor(Math.random()*16777215).toString(16);
   },
 
   getColors: function(e){
-                var colors = new Array();
-                for (var i = 0; i < this.grid.size; i++) {
-                  colors.push(this.grid.tiles[i].color);
-                }
-                return colors;
+    var colors = new Array();
+    for (var i = 0; i < this.grid.size; i++) {
+      colors.push(this.grid.tiles[i].color);
+    }
+    return colors;
   },
 
   makeNewColoredGrid: function(e){
-                    var colors = this.getColors(e);
-                    var grid = new Grid(colors.length);
-                    grid.name = ($("#formValueID").val());
-                    this.populateGrid(grid);
-                    for (var i = 0; i < colors.length; i++) {
-                      grid.tiles[i].color = colors[i];
-                    }
-                    return grid;
-
+    var colors = this.getColors(e);
+    var grid = new Grid(colors.length);
+    // Should probably be configurable
+    grid.name = ($("#formValueID").val());
+    this.populateGrid(grid);
+    for (var i = 0; i < colors.length; i++) {
+      grid.tiles[i].color = colors[i];
+    }
+    return grid;
   },
 
   saveGrid: function(e){
-                var grid = this.makeNewColoredGrid(e);
-                this.gridCollection.grids.push(grid);
-                this.view.appendSavedGrids(this.gridCollection);
-  },
-
+    var grid = this.makeNewColoredGrid(e);
+    this.gridCollection.grids.push(grid);
+    this.view.appendSavedGrids(this.gridCollection);
+  }
 }
-
-
-

source/js/src/models.js

@@ -1,5 +1,4 @@
-var ColorApp = {};
-
+/* Seem good */
 function GridCollection() {
   this.grids = new Array();
 }

source/js/src/view.js

@@ -4,34 +4,32 @@ ColorApp.View = function(opts){
 
 ColorApp.View.prototype = {
   displayDefaultGrid: function(grid){
-                      if (grid.tiles){
-                        this.updateGridImage(grid.tiles);
-                      }
+    if (grid.tiles){
+      this.updateGridImage(grid.tiles);
+    }
   },
 
   appendSavedGrids: function(gallery){
-                      var grids = gallery.grids;
-                      $(".saved-grids").html("");
-                      for (var i = grids.length - 1; i > 0; i--) {
-                        $(".saved-grids").append("<ul class=" + i + "><h2><b> Saved Grid: </b>" + grids[i].name + "</h2></ul>");
-                        this.updateGridImage(grids[i].tiles, ".saved-grids ul." + i);
-                      }
+    var grids = gallery.grids;
+    $(".saved-grids").html("");
+    for (var i = grids.length - 1; i > 0; i--) {
+      // Templates would be nice
+      $(".saved-grids").append("<ul class=" + i + "><h2><b> Saved Grid: </b>" + grids[i].name + "</h2></ul>");
+      this.updateGridImage(grids[i].tiles, ".saved-grids ul." + i);
+    }
   },
 
   updateGridImage: function(tiles, selector) {
-                      var gridSelector = selector || this.opts.gridSelector
-                      if (tiles) {
-                        for (var i = 0; i < tiles.length; i++) {
-                          $(gridSelector).append( "<li class=" + i + "></li>" );
-                          this.setColor(tiles[i], i, gridSelector);
-                        }
-                      }
+    var gridSelector = selector || this.opts.gridSelector
+    if (tiles) {
+      for (var i = 0; i < tiles.length; i++) {
+        $(gridSelector).append( "<li class=" + i + "></li>" );
+        this.setColor(tiles[i], i, gridSelector);
+      }
+    }
   },
 
   setColor: function(tile, index, gridSelector) {
-              $(gridSelector + " " + this.opts.tileSelector + "." + index).css("background-color", tile.color)
+    $(gridSelector + " " + this.opts.tileSelector + "." + index).css("background-color", tile.color)
   },
 }
-
-
-