6
\$\begingroup\$

Currently, the code below looks awful because of all the callbacks involved. How can I reduce the amount of nesting? Also, would you recommend the use of q or Async.js?

var mongoose = require('mongoose');
var User = require('../models/user.js');
var Genre = require('../models/genre.js');
var Album = require('../models/album.js');
var Band = require('../models/band.js');

exports.populate = function (req, res) {
  var bandNames = [
    'The Cool Group',
    'The Peeps',
    'Music Creation',
    'Number and Wumbers',
    'Testing For You',
    'What What',
    'Alrightly Then',
    'Is this the way it is?',
    'It hurts to know who you are',
    'There has been one or two of you',
    'Please and eyond'
  ];

  var userNames = [
    'syte',
    'koumarianos',
    'ericStas',
    'bloodCobia'
  ];

  albumId = Album.create({
      name: 'Test Album',
      biography: 'example'
    }, 
    function (err, album) {
      if (!err) {

        var albumId = album._id;

        Genre.create({ name: 'Rock' }, function (err, genre) {
            if (!err) {
              var genreId = genre._id;

              var users = [];

              for (var i = 0; i < userNames.length; i++) {
                users.push({
                  username: userNames[i]
                });
              }

              User.create(users, function (err) {
                var bands = [];
                var users = Array.prototype.slice.call(arguments, 1);
                var userIds = []

                for (var i = 0; i < users.length; i++) {
                  userIds.push(users[i]._id);
                }

                res.send(userIds);

                if (!err) {
                  for (var i = 0; i < bandNames.length; i++) {
                    bands.push({
                      name: bandNames[i],
                      biography: 'Lorem ipsum dolor sit amet, consectetur adipiscing elit. Aenean ac iaculis dui. Duis dignissim est ut ante cursus sodales. Duis et turpis ac nisl dictum mollis. Ut vel tempus metus. Vestibulum ante ipsum primis in faucibus orci luctus et ultrices posuere cubilia Curae; Duis eu risus aliquet, mattis est vitae, tempus sem. In eget ante semper, adipiscing arcu eu, fringilla lectus. Sed non tortor vulputate magna sodales varius eu quis nulla. Sed dignissim sed magna sit amet sodales. Ut nisi tortor, dapibus quis nulla eu, consectetur ornare dolor. Morbi mollis diam ut vulputate consectetur. In tincidunt, elit quis ultricies auctor, dolor odio condimentum ligula, quis sodales magna dui at est. Integer a orci rutrum, fermentum sapien quis, facilisis purus.',
                      albums: [albumId],
                      genres: [genreId],
                      members: userIds
                    });
                  }

                  // save all bands
                  Band.create(bands, function (err) {
                      if (!err) {
                        res.send("Successfully made all test data");
                      } else {
                        res.send(err);
                      }
                  });
                }
              });
            } else {
              res.send(err);
            }
          });
      } else {
        res.send(err);
      }
    });
};
\$\endgroup\$
  • \$\begingroup\$ Just to be clear: all those create methods take a callback because they create the item on the server and they execute the callback when it is done? \$\endgroup\$ – toto2 Sep 9 '13 at 14:11
1
\$\begingroup\$

My 2 cents are:

  • This code is understandable, but I would still rewrite it since it is cookie cutter, and you will surely move on to more complex stuff
  • Q uses a Promise object, Mongoose does not, so that would not work without some overhauling ( I personally like Q better though )
  • Async works with 'last parameter must be a callback' which is exactly how Mongoose works, so use that
|improve this answer|||||
\$\endgroup\$
1
\$\begingroup\$

I went ahead and refactored the code to use the async library. Apologies for it not being the exact same example above. This one included error handling, and the population of more test data, but the idea should be conveyed the same. :)

var mongoose = require('mongoose');
var User = require('../models/user.js');
var Genre = require('../models/genre.js');
var Album = require('../models/album.js');
var Song = require('../models/song.js');
var Band = require('../models/band.js');
var async = require('async');

exports.insertDummyData = function(req, res) {
mongoose.connect('mongodb://localhost/yourMusic');

var bandNames = [
  'The Cool Group',
  'The Peeps',
  'Music Creation',
  'Number and Wumbers',
  'Testing For You',
  'What What',
  'Alrightly Then',
  'Is this the way it is?',
  'It hurts to know who you are',
  'There has been one or two of you',
  'Please and eyond'
];

var songNames = [
  'stomy winds',
  'another one writes the series',
  'async breakdown',
  'dust in the php'
];

var userNames = [
  'syte',
  'koumarianos',
  'ericStas',
  'bloodCobia'
];

var genreNames = [
  'rap',
  'rock',
  'dance',
  'trance',
  'techno',
  'country'
];

var albumNames = [
    'Milk Money',
    'Rhyming No Dancing',
    'Touch Your Face',
    'A Sexual Reproduction',
    'The Spackle of the Wolf'
  ];

  var slice = Array.prototype.slice;

  async.auto({
    genres: function(callback) {
      var genres = [];

      for(var i = 0; i < genreNames.length; i++) {
        genres.push({ name: genreNames[i] });
      };

      Genre.create(genres, function(err) {
          var savedGenres = slice.call(arguments, 1);
            callback(err, savedGenres);
      });
    },
    users: function(callback) {
      var users = [];

      for (var i = 0; i < userNames.length; i++) {
        users.push({
          username: userNames[i]
        });
      }

      User.create(users, function(err) {
        var savedUsers = slice.call(arguments, 1);
        callback(null, savedUsers);
      });
    },
    songs: ['genres', function(callback, results) {
      var users = [];
      var songs = [];
      var genres = results.genres;
      var genre = genres[0];

      for (var i = 0; i < songNames.length; i++) {
        songs.push({
          name: songNames[i],
          genres: [genre]
        });
      }

      Song.create(songs, function(err) {
        var savedSongs = slice.call(arguments, 1);
        callback(err, savedSongs);
      });
    }],
    albums: ['songs', function(callback, results) {
      var albums = [];
      var songIds = [];
      var songs = results.songs;

      for(var i = 0; i < albumNames.length; i++) {
        albums.push({ name: albumNames[i], songs: songs });
      }

      Album.create(albums, function(err) {
        var savedAlbums = slice.call(arguments, 1);
        callback(null, savedAlbums);
      });
    }],
    bands: ['albums', 'users', function(callback, results) {
      var members = results.users;
      var songs = results.songs;
      var albums = results.albums;
      var bands = [];

      var bio = "this is a simple bio\
                  that i came up with.\
                  i hope it's good enough.";

      for(var i = 0; i < bandNames.length; i++) {
        bands.push({
          members: members,
          songs: songs,
          biography: bio,
          genres: [songs[0].genres],
          albums: albums
        })
      }

      Band.create(bands, function(err) {
        var savedBands = slice.call(arguments, 1);
        callback(null, savedBands);
      });
    }] 
  }, function(err, results) {
    if(err) {
      console.log(err);
      res.send(err)
    }
    else {
      res.send(results);
    }
  });
};
|improve this answer|||||
\$\endgroup\$
0
\$\begingroup\$
  1. The code does not look awful. It is normal async JavaScript.
  2. No, in this case I would not recommend to to use Async, step or sth. else.
  3. When you have got used to async programming, you will understand 1. and 2.
|improve this answer|||||
\$\endgroup\$
  • 3
    \$\begingroup\$ I disagree with that first statement. Any code with so many indentation levels needs refactoring. \$\endgroup\$ – Changaco Sep 9 '13 at 13:04
  • \$\begingroup\$ I disagree with your statement @Changaco. The quality of code has no relation to the amount of indentation levels. \$\endgroup\$ – Jonny Sooter Sep 10 '13 at 14:29
  • 1
    \$\begingroup\$ "It is normal async JavaScript" that doesn't mean it isn't awful looking. Promise objects are probably the best the asker is going to get without switching languages. \$\endgroup\$ – Wesley Wiser Sep 10 '13 at 14:41
  • \$\begingroup\$ @JonnySooter Except when you decide that you need to add some steps, change the order in which they happen, or make any change which would be trivial if callback nesting didn't tangle everything up and make things difficult to modify and easy to break :P \$\endgroup\$ – shambulator Mar 10 '14 at 15:23

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.