Code review notes

browser/js/common/directives/styling/styling-selector.directive.js

@@ -5,16 +5,16 @@ app.directive("stylingSelector", function(){
     link: function(scope, element, attrs, ngClick){
         scope.getElementToStyle = function(id){
           var parentEl = document.getElementById(""+ id +"");
-          console.log("CURRENT ID:", parentEl, id);
+          console.log("CURRENT ID:", parentEl, id); // @OB/ND dead code
           var elementToStyle = $(parentEl).find(".lasso-user-content").children().first();
-          console.log(elementToStyle);
+          console.log(elementToStyle); // @OB/ND dead code
           if(elementToStyle.length > 0) {
             console.log(elementToStyle);
 
             scope.styleGroup.push(elementToStyle);
           }
 
-          console.log(scope.styleGroup);
+          console.log(scope.styleGroup); // @OB/ND dead code
         }
     }
   }

browser/js/common/factories/GridFactory.js

@@ -28,6 +28,7 @@ app.factory('GridFactory', function($http, $compile, PageFactory, ProjectFactory
     // helper function to create a new element
     GridFactory.createElement = function(scope, id, content) {
         var content = content || "Your content here";
+        // @OB/ND this should almost certainly be a directive
         var el = $compile("<div class='grid-stack-item' id=" +
             id + "><div class='grid-stack-item-content new-element container'>\
   <div class='row'>\
@@ -73,8 +74,8 @@ app.factory('GridFactory', function($http, $compile, PageFactory, ProjectFactory
         // add an Add Widget Button to the newly nested grid
         $("#" + id + " .lasso-button-box")
             .append($compile("<button ng-click='addNewGridElement(nestedGrids." + newGridID + ")'>Add Widget</button>")(scope));
-         console.log("newgridid", newGridID);   
-         console.log("nested grids:", GridFactory.nestedGrids);
+         console.log("newgridid", newGridID); // @OB/ND dead code
+         console.log("nested grids:", GridFactory.nestedGrids); // @OB/ND dead code
     }
 
     GridFactory.removeWidget = function(idNum) {

browser/js/common/factories/PageFactory.js

@@ -6,7 +6,7 @@ app.factory('PageFactory', function($rootScope, $http){
         .then(function(page){
             return page.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -15,7 +15,7 @@ app.factory('PageFactory', function($rootScope, $http){
         .then(function(pages){
             return pages.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -24,7 +24,7 @@ app.factory('PageFactory', function($rootScope, $http){
         .then(function(page){
             return page.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -33,7 +33,7 @@ app.factory('PageFactory', function($rootScope, $http){
         .then(function(page){
             return page.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -42,7 +42,7 @@ app.factory('PageFactory', function($rootScope, $http){
         .then(function(page){
             return page.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 

browser/js/common/factories/ProjectFactory.js

@@ -7,7 +7,7 @@ app.factory('ProjectFactory', function($http){
         .then(function(project){
             return project.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -16,7 +16,7 @@ app.factory('ProjectFactory', function($http){
         .then(function(project){
             return project.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -25,7 +25,7 @@ app.factory('ProjectFactory', function($http){
         .then(function(project){
             return project.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -34,7 +34,7 @@ app.factory('ProjectFactory', function($http){
         .then(function(project){
             return project.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 

browser/js/common/factories/UserFactory.js

@@ -6,7 +6,7 @@ app.factory('UserFactory', function($rootScope, $http){
         .then(function(user){
             return user.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -15,7 +15,7 @@ app.factory('UserFactory', function($rootScope, $http){
         .then(function(users){
             return users.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -24,7 +24,7 @@ app.factory('UserFactory', function($rootScope, $http){
         .then(function(user){
             return user.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -33,7 +33,7 @@ app.factory('UserFactory', function($rootScope, $http){
         .then(function(user){
             return user.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 
@@ -42,7 +42,7 @@ app.factory('UserFactory', function($rootScope, $http){
         .then(function(user){
             return user.data
         }, function(err){
-            return err;
+            return err; // @OB/ND beware!
         })
     }
 

browser/js/create-layout/create-layout-controller.js

@@ -1,5 +1,6 @@
 app.controller("CreateLayoutCtrl", function($scope, $compile, AuthService, GridCompFactory, GridFactory) {
 
+    // @OB/ND could use resolve instead
     AuthService.getLoggedInUser().then(function (user) {
         if(user) {
             $scope.user = user;
@@ -11,13 +12,14 @@ app.controller("CreateLayoutCtrl", function($scope, $compile, AuthService, GridC
     $scope.nestedGrids = GridFactory.getNestedGrids();
 
     $scope.addNewGridElement = function (grid, content){
-      GridFactory.addNewGridElement($scope, grid, content);
+      GridFactory.addNewGridElement($scope, grid, content); // @OB/ND we don't think you need to pass scope through (though you'd have to refactor the factory method)
     }
 
     $scope.addNestedGrid = function(id) {
        GridFactory.addNestedGrid($scope, id);
     }
 
+    // @OB/ND $scope.removeWidget = GridFactory.removeWidget
     $scope.removeWidget = function(idNum) {
        GridFactory.removeWidget(idNum);
     }

server/app/routes/pages/index.js

@@ -2,12 +2,12 @@
 var router = require('express').Router();
 module.exports = router;
 
-var Page = require('../../../db/models/page.js');
+var Page = require('../../../db/models/page.js'); // @OB/ND alternative is to do mongoose.model('Project')
 
 
 
 router.param('id', function (req, res, next, id){
-	req.id = req.params.id;
+	req.id = req.params.id; // @OB/ND does not seem all that useful
 	next();
 })
 
@@ -22,15 +22,15 @@ router.get('/', function (req, res, next){
 router.get('/:id', function (req, res, next){
 	Page.findById(req.id)
 	.then(function ( page ){
-		res.status(201).send( page );
+		res.status(201).send( page ); // @OB/ND 201 is non-standard here
 	})
 	.then(null, next);
 })
 
 router.put('/:id', function (req, res, next){
 	Page.findByIdAndUpdate(req.id, req.body, {new: true})
 	.then(function ( page ){
-		res.status(201).send( page );
+		res.status(201).send( page ); // @OB/ND 201 is non-standard here
 	})
 	.then(null, next);
 })

server/app/routes/projects/index.js

@@ -2,10 +2,10 @@
 var router = require('express').Router();
 module.exports = router;
 
-var Project = require('../../../db/models/project.js');
-
+var Project = require('../../../db/models/project.js'); // @OB/ND alternative is to do mongoose.model('Project')
 
 
+// @OB/ND dead code below...though actually why not use a router.param
 // router.param('id', function (req, res, next, id){
 // 	req.id = req.params.id;
 // 	next();
@@ -22,15 +22,15 @@ router.get('/', function (req, res, next){
 router.get('/:id', function (req, res, next){
 	Project.findById( req.params.id)
 	.then(function ( project ){
-		res.status(201).send( project );
+		res.status(201).send( project ); // @OB/ND 201 is non-standard here
 	})
 	.then(null, next);
 })
 
 router.put('/:id', function (req, res, next){
-	Project.findByIdAndUpdate( req.params.id, req.body, {new: true})
+	Project.findByIdAndUpdate( req.params.id, req.body, {new: true}) // @OB/ND some hooks won't fire
 	.then(function ( project ){
-		res.status(201).send( project );
+		res.status(201).send( project ); // @OB/ND 201 is non-standard here
 	})
 	.then(null, next);
 })
@@ -41,7 +41,7 @@ router.post('/', function (req, res, next){
 		res.status(201).send(project);
 	})
 	.then(null, function(err){
-		console.log("in post project router fail")
+		console.log("in post project router fail") // @OB/ND dead code
 		err.status = 400;
 		next(err);
 	})

server/db/models/page.js

@@ -4,10 +4,10 @@ var ObjectId = mongoose.Schema.Types.ObjectId;
 
 var PageSchema = new mongoose.Schema({
 	project: {type: ObjectId, ref: "Project", required: true},
-	name: { type: String, unique: true, required: true},
+	name: { type: String, unique: true, required: true}, // @OB/ND could be problematic
 	html: String,
 	css: String,
-	grid: {}
+	grid: {} // @OB/ND what is this for?
 })
 
 module.exports = mongoose.model('Page', PageSchema);