3
\$\begingroup\$

I would like to gather ideas on how to refactor the JavaScript/NodeJS controller code below to be more aesthetic. The code is using the koa framework thus I use yield commands. I also use mongoose (I'm waiting for 3.9.2 which will support promises).

As a Ruby on Rails developer, I'm searching for a Rails-style way on writing NodeJS programs.

'use strict'

/*
 * Model dependencies
 */

var _ = require('lodash')
var parse = require('co-body')
var db = require('mongoose-glue')

/*
 * User controller.
 */

module.exports = {

  /*
   * Lists available users.
   */

  index: function*(id) {
    var skip, limit
    skip = Math.max(0, this.query.skip)
    limit = Math.min(10, this.query.limit)
    this.body = yield db.model('user').find().skip(skip).limit(limit).exec()
  },

  /*
   * Displays user with an id.
   */

  show: function*(id) {
    this.body = yield db.model('user').findById(id).exec()
    this.status = this.body ? 200 : 404
  },

  /*
   * Creates a new user.
   */

  create: function*() {
    var data
    try {
      data = yield formData(this, 'name')
      this.body = yield createUser(data)
      this.status = 201
    } catch(e) {
      this.body = e
      this.status = 403
    }
  },

  /*
   * Updates user with an id.
   */

  update: function*(id) {
    var data
    try {
      data = yield formData(this, 'name')
      this.body = yield updateUser(id, data)
    } catch(e) {
      this.body = e
      this.status = 403
    }
  },

  /*
   * Removes user with an id.
   */

  destroy: function*(id) {
    var item
    try {
      item = yield destroyUser(id)
      this.status = item ? 200 : 404
    } catch(e) {
      this.body = e
      this.status = 403
    }
  },

}

/*
 * Create user thunk
 */

function createUser(data) {
  return function(next) {
    db.model('user').create(data, next)
  }
}

/*
 * Update user thunk.
 */

function updateUser(id, data) {
  return function(next) {
    db.model('user').findByIdAndUpdate(id, data, next)
  }
}

/*
 * Create user thunk
 */

function destroyUser(id) {
  return function(next) {
    db.model('user').findByIdAndRemove(id, next)
  }
}

/*
 * Returns form data.
 */

function *formData(ctx) {
  var data
  data = yield parse.form(ctx)
  data = _.any(arguments) ? _.pick(data, arguments) : data
  return data
}
\$\endgroup\$
1
\$\begingroup\$

Interesting question,

I understand you search for a rails-style way on how to write nodejs programs. However from a CodeReview perspective, your code should be idiomatic (conforming to the mode of expression characteristic of a language) so that other developers can grok your code and even maintain it.

From that perspective your code is quite good, the only thing being that you really (really) should put those semi-colons. Just get over it, good JavaScript has semicolons at the end of each line.

Other than that:

  • I like the use of 'use strict', generator functions and yield
  • I like the amount of comments, not so much the style, I would go for less vertical real estate

    /*
     * User controller.
     */
    

    could just as well be

    //User controller
    
  • It's a matter of taste but I find this more idiomatic

    var skip = Math.max(0, this.query.skip), 
        limit = Math.min(10, this.query.limit);
    

    than

    var skip, limit
    skip = Math.max(0, this.query.skip)
    limit = Math.min(10, this.query.limit)
    

    I guess from a style perspective you want to split declaration from initialization, but I find this takes too much vertical real estate.

  • Logging/Handling errors, you should consider putting more effort in handling errors than simply throwing a 403 on the client side, you should do something on the server side as well

  • Magic constants (404, 200) etc. are known well enough that you don't have to create named constants.

All in all I like your code, I guess if I had to maintain I would run it thru a script to fix all the semicolons and comments and go from there ;)

\$\endgroup\$
  • 1
    \$\begingroup\$ Thx @konijn for your proposals. Btw, I always write semicolons but for this piece of code I tried to write things more like with style of coffescript or rails. I would appreciate ideas on how to optimize mongoose queries. I was thinking to move that kind of logic into a service class. \$\endgroup\$ – xpepermint Sep 8 '14 at 16:56

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.