Take the 2-minute tour ×
Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

I'm building a media viewer web app for a kiosk in node that uses data in mongo. To render the viewer, it gathers together all the Asset objects (representing video files), each of which belong to a Category, each of which belong to a Medium.

This is my first time using mongo and node together, so I decided to use async.js to organize the code, but I don't know if I'm doing things efficiently or not. Is there a better way to organize the data or perform the queries? Does this look remotely sane?

Additionally, I commented out some code I thought looked correct but would mysteriously overwrite the object simply by adding another key to it. What am I doing wrong?

models.js : mongoose models

var mongoose = require('mongoose');

/* Mediums... Film, Video, TV */
var mediumSchema = mongoose.Schema({
    name: String,
    id: String,
    description: String
});
var Medium = mongoose.model('mediums', mediumSchema);

/* Categories ... */
var categorySchema = mongoose.Schema({
    medium: String, // refers to medium.id, not medium._id
    name: String,
    shortName: String,
    years: String,
    description: String
});
var Category = mongoose.model('categories', categorySchema);

/* Assets */
var assetSchema = mongoose.Schema({
    title: String
,   year: Number
,   duration: Number
,   medium_info: String
,   copyright: String
,   extended_info: String
,   filename: String
,   category_id: mongoose.Schema.Types.ObjectId
});
var Asset = mongoose.model('assets', assetSchema);

exports.Medium = Medium;
exports.Category = Category;
exports.Asset = Asset;

viewer.js : the handler that retrieves all the data and renders the html

exports.index = function(req, res){
  res.render('index');
};

exports.viewer = function(req, res) {
  var models = require('./models');
  var async = require("async");
  var mongoose = require('mongoose');
  mongoose.connect('mongodb://localhost/test');

  var db = mongoose.connection;
  db.on('error', console.error.bind(console, 'connection error:'));
  db.once('open', function callback () {});

  // provide category object, returns assets in that category
  function getAssetsForCategory(category, callback) {
    console.log("Looking for assets for category:" + category.name);
    models.Asset.find({category_id: category._id}, function(err, assets) {
      callback(err, assets);
    });
  }
  // provide medium label ("FILM","VIDEO","TV") returns list of categories
  function getCategoriesForMedium(medium, callback) {
    console.log("finding categories for " + medium);
    models.Category.find({medium: medium}, function(err, categories) {
      callback(err, categories);
    });
  }

  data = {};
  async.each(
    ["FILM","VIDEO","TV"],
    function(mediumName, mediumLookupComplete) {

      var mediumResults = {};
      var categoryResults = {};
      var assetResults = {};
      async.waterfall(
        [
          function(findMediumComplete) {
            // get medium object for its additional metadata
            models.Medium.findOne({id: mediumName}, findMediumComplete)
          },

          function(medium, findCategoriesComplete) {
            mediumResults.name = medium.name;
            mediumResults.description = medium.description;
            getCategoriesForMedium(medium.id, findCategoriesComplete);
          },

          function(categories, assetFetchComplete) {
            async.each(
              categories,
              function(category, categoryAssetLookupDone) {
                categoryResults[category._id] = category;
                /* I originally wanted to just insert the list of assets
                   into their appropriate category object here, but doing so
                   would overwrite the entire object--explanations? */
                // categoryResults[category._id].assets = [];
                getAssetsForCategory(category, function(err, assets) {
                  /* instead I have to use a separate temp object to store the
                     asset results, rather than just saying
                     categoryResults[category._id].assets = assets; */
                  assetResults[category._id] = assets;
                  categoryAssetLookupDone();
                });
              },
              assetFetchComplete
            );
          }
        ],
        function(err) {
          console.log("asset fetch complete");
          data[mediumName] = {};
          data[mediumName]["categories"]  = categoryResults;
          data[mediumName]["description"] = mediumResults.description;
          data[mediumName]["name"] = mediumResults.name;

          data[mediumName].categories = [];
          for (var catId in assetResults) {
            data[mediumName].categories.push({info: categoryResults[catId], assets: assetResults[catId]});
            //data[mediumName]["categories"][catId] = {info: data[mediumName].categories[catId], assets: assetResults[catId]};
          }
          //data[mediumName]["assets"] = assetResults;
          //{medium: mediumResults, ass: assetResults};
          mediumLookupComplete(err);
        }
      );
    },
    function(err) {
      mongoose.connection.close();
      //res.send({data:data});
      res.render('viewer', {data:data});
    }
  );
};
share|improve this question
    
Welcome to Code Review! Your question looks good to me if your code work as is, without the commented code. We don't fix broken code, since your question is asking for a review it looks on-topic to me. –  Marc-Andre Mar 19 at 18:42
    
Yes, it works as provided, just seems convoluted. Thanks! –  Luc Mar 20 at 19:08
    
Thanks you for the confirmation! I hope you'll get good review! –  Marc-Andre Mar 20 at 19:10
    
Instead of using the _id of the other objects you could use a reference. See mongoosejs.com/docs/populate.html Then it would allow you to populate the fields –  jackdbernier Jun 2 at 13:40
add comment

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

Browse other questions tagged or ask your own question.