4
\$\begingroup\$

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});
    }
  );
};
\$\endgroup\$
  • \$\begingroup\$ 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. \$\endgroup\$ – Marc-Andre Mar 19 '14 at 18:42
  • \$\begingroup\$ Yes, it works as provided, just seems convoluted. Thanks! \$\endgroup\$ – Luc Mar 20 '14 at 19:08
  • \$\begingroup\$ Thanks you for the confirmation! I hope you'll get good review! \$\endgroup\$ – Marc-Andre Mar 20 '14 at 19:10
  • \$\begingroup\$ 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 \$\endgroup\$ – jackdbernier Jun 2 '14 at 13:40
5
\$\begingroup\$

A short review;

  • Consider using JsHint
  • Declare data with var, otherwise you are polluting the global namespace
  • Consider err more often, the assumption that there will be no error will bite you
  • Do not simply log to console.log, it is one of the most common bottlenecks
  • Remove your commented out code, it keeps things clean
  • My assumption is that

    • The meta data of ["FILM","VIDEO","TV"] does not change often
    • The categories do no change often

    I would generate a javascript file with the meta data and categories in 1 big Object Notation assignment, and re-generate it whenever mediums/categories change, this would greatly simplify your code, and make it way faster.

  • With Object Notation, this

      data[mediumName] = {};
      data[mediumName]["categories"]  = categoryResults;
      data[mediumName]["description"] = mediumResults.description;
      data[mediumName]["name"] = mediumResults.name;
    

    would be

      data[mediumName] = {
        categories  : [];
        description : mediumResults.description;
        name : mediumResults.name;
      };
    

    note that you override the results of data[mediumName]["categories"] = categoryResults; with data[mediumName].categories = [];

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.