Homework 4 feedback

lessons/04-mongo/ZG_persistentCatsApp/routes/index.js

@@ -50,17 +50,40 @@ var create = function(req, res){
 //del function is for the /cats/delete/old route, sorts the cats so the oldest is the first one,
 //and deletes it
 var del = function(req, res){
+	// Nice use of Mongoose sort!
 	Cat.findOneAndRemove({}, {sort: {age: -1}}, function(err, cat){	//sorts in descending order by age so that the first one is the oldest
 		if (err){
 			console.log("Error: ", err);
 		} else {
-			console.log("cat", cat);
+			var data;
 			if (cat){
-				var data = {cat: cat};
-				data['context'] = 'Say goodbye to your oldest cat!';
+				data = {
+					cat: cat,
+					context: 'Say goodbye to your oldest cat!'
+				};
+				// I wouldn't define one property in your literal object definition and 
+				// another with bracket notation -- inconsistent. Just go ahead and put
+				// both in the literal object definition, like this
 			} else {
-				var data = {context: 'You do not have any cats to send to the farm right now. Go and get some!'}
+				data = {
+					context: 'You do not have any cats to send to the farm right now. Go and get some!'
+				};
 			}
+			/*
+			Usually if you're defining the same variable one way in an if and another way in an else,
+			it's best practice to put the "var data" outside the if/else instead of inside both the if and the else.
+
+			This would also be a great place to use the ternary logic I mentioned in your HW3 feedback:
+
+			var data = cat ? {
+				cat: cat,
+				context: 'Say goodbye to your oldest cat!'
+			} : {
+				context: 'You do not have any cats to send...'
+			};
+
+			Just a little more compact.
+			*/
 			res.render('indiv', data);
 		}
 	});
@@ -80,48 +103,73 @@ var sortColor = function(req, res){
 //sortName function for the /cats/byname/:name route, gets whatever letters the user has inputted and displays
 //all the cats whose names start with those letters and sorts them by age
 var sortName = function(req, res){
-	var filteredName = req.params.name.toString();
+	var filteredName = req.params.name.toString(); // pretty sure req.params.name is already a string
 
 	var regexp = new RegExp("^"+ req.params.name);
+	// You don't need a regexp for the Mongoose find method -- can just use 
+	// {name: req.params.name} as your query
 
-	console.log(filteredName);
 	Cat.find({name: regexp}, function(err, cats){
 		if (err){
 			console.log("Error: ", err);
 		} else {
-			var data = {cats: cats};
-			data['context'] = 'Here are all of your cats whose names start with ' + filteredName;
+			var data = {
+				cats: cats,
+				context: 'Here are all of your cats whose names start with ' + filteredName
+			};
+			// Again, don't mix two ways to put properties on an object -- inconsistent
 			res.render('list', data);
 		}
 	}).sort({age: 1});
+	// Usually you see .find(query).sort(params).exec(function() {...}),
+	// instead of the .sort at the end like this. See the answers here:
+	// http://stackoverflow.com/questions/5825520/in-mongoose-how-do-i-sort-by-date-node-js
 };
 
 //sortAge function for the /cats/byage/:age route, gets age range user inputs and displays and sorts by age all cats
 //in that range. Based on how this function is written, user can input a single age and will see all cats above that age,
 //they can input the age range with a hyphen between the numbers, or they can input "-MaxAge" which will show all cats
 //below that age or, put another way, between the ages of 0 and the max age.
 var sortAge = function(req, res){
-	var filteredAge = req.params.age.toString();
+	var filteredAge = req.params.age.toString(); // are you sure it isn't already a string?
 	var ageArr = filteredAge.split("-");
-	console.log(ageArr);
+	// would need more complex logic here to handle the case where req.params.age is something like "-42", yes?
+	// You mentioned you're handling all one-number cases as "all cats older than ___" -- I bet you could have figured out how to handle the two cases
 
-	if (ageArr.length === 2){
-		Cat.find({$and: [{age: {$gte: Number(ageArr[0])}}, {age: {$lte: Number(ageArr[1])}}]}, function(err, cats){
-			var data = {cats: cats};
-			data['context'] = 'Here are all of your cats between ages ' + ageArr[0] + ' and ' + ageArr[1];
-			res.render('list', data);
-		}).sort({age:1});
-	} else if (ageArr.length === 1){
-		Cat.find({age: {$gte: Number(ageArr[0])}}, function(err, cats){
-			var data = {cats: cats};
-			data['context'] = 'Here are all of your cats above age ' + ageArr[0];
-			res.render('list', data);
-		}).sort({age:1});
+
+	// Your query is complicated enough that I might pull it out and construct it separately. More readable this way.
+	// Also, I restructured to be less repetitive -- this is one way you could do things
+	var query;
+	var context;
+	if (ageArr.length === 2) {
+		context = 'Here are all of your cats between ages ' + ageArr[0] + ' and ' + ageArr[1];
+		query = {
+			$and: [
+				{age: {$gte: Number(ageArr[0])}}, 
+				{age: {$lte: Number(ageArr[1])}}
+			]
+		};
+	} else if (ageArr.length === 1) {
+		context = 'Here are all of your cats above age ' + ageArr[0];
+		query = {
+			age: {$gte: Number(ageArr[0])}
+		};
 	}
+
+	// again -- .find, .sort, .exec is the usual pattern
+	Cat.find(query).sort({age:1}).exec(function(err, cats) {
+		res.render('list', {
+			cats: cats,
+			context: context
+		});
+	});
 };
 
 //all these export statements are annoying but the only other thing I've seen is exporting an object whose keys correspond 
 //to values that are functions
+// Much better to export an object whose keys correspond to values that are functions! See HW3 feedback.
+// Really, that's what you're doing here, just in more lines of code. The object is module.exports,
+// the keys are cats, create, del, sortColor... etc
 module.exports.cats = cats;
 module.exports.create = create;
 module.exports.del = del;