Express.js controller with Node.js style exports.methods

Question

I have some Node/Express.js controllers which implement the exports.method Node module convention. This particular controller also has a few helper methods. The code works fine!

However, I'm a stickler for conventions, best practices etc. and recently I've been going back over one of my project with jshint (Sublime Text 3) and noticed my controllers throw up lots of complaints. Well really it's the same complaint over and over again - that methods called inside a exports closure are not defined, they are in fact defined in the controller but outside of the exports closure so they can be reused.

Example, jshint complaint

• 9:14 'parseQueryString' is not defined

So really what I'm asking, is how do you recommend I structure my controller (and also more generally node modules that use exports closures) for correctness / best practice?

Here's the code for your reference:

/* global require, exports */
'use strict';

var mongoose = require('mongoose'),
    Time = mongoose.model('Time');

exports.findAll = function (req, res) {

    var qs = parseQueryString(req);

    // Importantly we should only query times for current user.
    qs.query.userId = getUserVal(req, '_id');

    log(req, 'Retrieving all times');
    Time.find(qs.query, qs.fields, qs.options, function (err, result) {
        if (err) { return handleError(req, res, err); }
        res.send(result);
    });
};

exports.findById = function (req, res) {

    var id = req.params.id,
        userId = req.user.get('_id');

    log(req, 'Retrieving time: ' + id);
    Time.findOne({'_id':id,'userId':userId}, function (err, result) {
        if (err) { return handleError(req, res, err); }
        res.send(result);
    });
};

exports.addTime = function (req, res) {

    var time = new Time(req.body);

    // Enforce user association
    time.userId = req.user.get('_id');
    time.user = req.user.get('name');

    log(req, 'Adding time: ' + time);
    time.save(function (err) {
        if (err) { return handleError(req, res, err); }
        Time.findById(time._id, function (err, found) {
            time = found;
        });
        res.send(time);
    });
};

exports.updateTime = function (req, res) {

    var id = req.params.id,
        userId = req.user.get('_id'),
        time = req.body;
    delete time._id;

    log(req, 'Updating time: ' + id);
    Time.update({'_id':id,'userId':userId}, time, function (err, rowsAffected, raw) {
        if (err) { return handleError(req, res, err); }
        console.log('The number of updated documents was %d', rowsAffected);
        console.log('The raw response from Mongo was ', raw);
    });
};

exports.deleteTime = function (req, res) {

    var id = req.params.id,
        userId = req.user.get('_id');

    log(req, 'Deleting time: ' + id);
    Time.remove({'_id':id,'userId':userId}, function (err) {
        if (err) { return handleError(req, res, err); }
        res.send(req.body);
    });
};


// Helpers

handleError = function(req, res, err) {
    log(req, 'error : ' + err);
    res.send({'error' : err});
};

log = function(req, msg) {
    console.log(msg + ' | u:' + getUserVal(req, '_id'));
};

getUserVal = function(req, val) {
    return req.user ? req.user.get(val) : '';
};

// Extract options from query string 
// and format for mongoose query as follows:
// query = {'client':'ABK','category':'Email'}
// fields = 'client category'
// options = {limit:10,skip:0}
parseQueryString = function(req) {

    var result = {
        query: JSON.parse(JSON.stringify(req.query)),
        fields: '',
        options: {}
    },
    tmp = result.query;

    for (var key in tmp) {
        if (tmp.hasOwnProperty(key)) {
            var val = tmp[key];
            // console.log(' + key + ' = ' + val);
            if (key === ':show') {
                result.fields = val.replace(',', ' ');
                delete result.query[key];
            }
            else if (key === ':limit') {
                result.options.limit = parseInt(val);
                delete result.query[key];
            }
            else if (key === ':skip') {
                result.options.skip = parseInt(val);
                delete result.query[key];
            }
        }
    }
    // console.log('result = ' + JSON.stringify(result));
    return result;
};

Answer 1

Like I mentioned in my comment, adding var will probably not solve the problem. The parseQueryString function is declared as a variable. Therefor it should be placed earlier in the file than the call to that function. Like explained here https://stackoverflow.com/a/336868/327004. There is 2 options. First, you declare your function like this function parseQueryString() {...} or you move it at the top of the file.

Also, it's a code review platform and the title starts with "How to improve...".

Here's some comments.

parseQueryString by its name I think it will return an object containing all supported parameters present in the querystring. Though, by the comments I know it does way more. It also create a mongoose query. "Extract options from query AND format for mongoose query". Second, this function does more then one thing. Therefor, you should split it in 2. I would have 2 or 3 functions (the second one is optional) extractParametersFromQueryString, applyDefaultValues and createMongooseQuery.

Finally has mentioned by @Jivings, add the varkeyword.