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 learning Node. I'm working on a personal project that uses Node + Express + Node-MySQL + Generic-Pool (for MySQL pooling). I am new to the idea of module.exports and I'm still grokking it.

The following code works. However I want to organize my code in a way that is logical and optimal, and MVCish. I am wondering if those of you who have done this before could take a look at the following and let me know if this is the "right" way to structure things. (Yes I know this going to depend a lot on the developer. But at least knowing if there are obvious problems in my approach would be helpful.)

My folder structure looks roughly like this:

app.js
routes/
models/
base/

In my main app.js file I have...

var express = require( "express" ),
    mysql = require( "mysql" ),
    generic_pool = require ( "generic-pool" ),
    routes = require( "./routes" );

var app = module.exports = express(),
    pool = generic_pool.Pool({
        name: "mysql",
        create: function( cb ) {
            var db = mysql.createConnection( require( "./config/db" ) );
            db.connect();
            cb( null, db );
        },
        destroy: function( db ) {
            db.end();
        },
    });

app.configure(function(){
    app.set( "pool", pool );
    ...
});
routes( app );
app.listen(80, function());

Here is an example route/ file "stuff.js"

module.exports = function( app ){
    var model = require( "../models/stuff" );
    model.init( app );
    app.get( "/stuff", function( req, res ){
        model.get( function( result ){
            res.json( result );
        });
    });
};

Here is an example model/ file "stuff.js"

var db;
module.exports.init = function( app ){
    db = require( "../base/db" );
    db.init( app );
};
module.exports.get = function( cb ){
    var sql = "SELECT * FROM stuff";
    db.do( sql, false, cb );
};

And here is the base/db file

var pool;
module.exports.init = function( app ){
    pool = app.get( "pool" );
};
module.exports.do = function( sql, params, cb ){
    pool.acquire(function( err, db ){
        if (err) {
            // ...
        } else {
            db.query( sql, function( err, result ) {
                if ( err ) {
                    // ...
                } else if ( typeof cb === "function" ) {
                    cb( result );
                }
            });
        }
        pool.release( "db" );
    });
};
share|improve this question
1  
what is the last line of app.js supposed to do? It lacks either an identifier to replace function(), or the actual body of the function (with the required arguments) –  toasted_flakes Dec 20 '13 at 18:38

1 Answer 1

Your code looks fine.

However, you are not showing us much to review, definitely come back when there is more to review.

Some minor comments:

  • Having your db connection config through ./config/db is good
  • You probably want to throw an exception if ( typeof cb !== "function" ) instead of silently ignoring that
  • The base/db code nests 4 levels of callback, you should look into queue management libraries for node.js
share|improve this answer

Your Answer

 
discard

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

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