Re: [nodejs] Please provide feedback as to best practice for reading files from a specific directory

[Martin Cooper]

A few things:

* You're loading all of the files each time you process a request. Assuming you want to load them on startup only, and make sure they're loaded before you start handling requests, you should do the loading in your mainline code, before you call listen. Something like:

    loadTemplates(function (err) {

        // Check err and bail out if something bad happened.

        // App Listen

        app.listen(3000);

        console.log('Listening on port 3000');

    });

* The fs.readdir() and fs.readFile() functions are async. You should never be throwing from their callbacks, since you won't be around (in the call stack) to catch them. You need to be calling your own callback with any errors instead.

* Since your readDir() function will complete asynchronously, its work is not yet complete when you call res.send(). (Addressing the first point above will address this, though.)

* You should use the 'path' module to resolve the paths to your directories and files, instead of gluing them together as strings.

Hope that helps.

--

Martin Cooper

On Fri, Nov 9, 2012 at 9:56 PM, Chris Buttery <but...@gmail.com> wrote:

Hi Everyone, I'm a long time reader, first time poster.

I'm wondering if you guys could provide some feedback on this code I've written, for my first attempt at an app.

My intention is to read the contents of files from a specific directory.
So when you load this 'app' it iterates all the files in 'templates' and sends them to my readFile() function to be read.

This script works as I expected, but I'd like to see how some of your more experienced developers would have attempted this.

Thanks

Chris

// Set up the app

var express = require('express')

    , app = express()

    , fs = require('fs');

// Define the path to the templates and set it to 'dir'

app.set('templates', __dirname + '/templates');

var dir = app.get('templates');

// Get all files from the template dir and

// send em to readFile() to be read

var readDir = function(){

    fs.readdir(dir, function (err,files){

if(err) throw err;

        files.forEach(function(file) {

            readFile(file);

        });

    });

};

// Read the files

var readFile = function (file) {

    fs.readFile(dir+'/'+file, 'utf8', function (err,data) {

        if(err) throw err;

        console.log("read this " + data);

    });

};

// Default route

app.get('/', function(req, res){

    readDir();

    res.send('Styleguide');

});

// App Listen

app.listen(3000);

console.log('Listening on port 3000');

--
Job Board: http://jobs.nodejs.org/
Posting guidelines: https://github.com/joyent/node/wiki/Mailing-List-Posting-Guidelines
You received this message because you are subscribed to the Google
Groups "nodejs" group.
To post to this group, send email to nod...@googlegroups.com
To unsubscribe from this group, send email to
nodejs+un...@googlegroups.com
For more options, visit this group at
http://groups.google.com/group/nodejs?hl=en?hl=en

[kinghokum]

Thanks Martin, I appreciate it.
I'll look into how to implement your suggestions.

[kinghokum]

Hi all, 

Just wondering if this is any better.
I've tried to implement what Martin has suggested best to my knowledge.

Any further feedback is appreciated.
Thanks

// Set up the app

var express = require('express')

    , app = express()

    , fs = require('fs')

    , path = require('path');

// Define the path to the templates and set it to 'dir'

var url = path.resolve('./templates/');

// Get all files from the template dir and send them to readFile() to be read

var readDir = function(){

    fs.readdir(url, function (err,files){

        if (err) {

            console.log(err.message);

            return;

        }

        // for each of the files...

        files.forEach(function (file) {

            var filePath = path.join(url, file);

            // Get a files stats

            fs.stat(filePath, function (err, stat) {

                // If the file is not a directory, read it

                if (stat && stat.isFile()) {

                    fs.readFile(filePath, 'utf8', function (err,data) {

                        if (err) {

                            console.log(err.message);

                            return;

                        }

                        console.log('read this ' + data);

                    });

                }

            });

        });

        // Assuming the readdir() is complete, lets call templatesLoaded();

        templatesLoaded(err);

    });

};

// when readdir() is complete, call templatesLoaded()

var templatesLoaded = function (err) {

    // Check err and bail out if something bad happened.

    if (err) {

        console.log(err.message);

        return;

    }

    app.listen(3001);

    console.log('Listening on port 3000');

};

// Default route

app.get('/', function(req, res){

    res.send('Styleguide');

});

// Call readDir();

readDir();

[Martin Cooper]

You were doing fine right up until that "assuming". :-) At that point, you've kicked off the process of reading in the template files, but that has not yet completed. It's only complete when the last (in time, but not necessarily in sequence) fs.readFile() call has called its callback. There are lots of options for managing this, but if it was my code, I'd be using async.forEach(), instead of the raw JavaScript forEach:

https://github.com/caolan/async#forEach

Something like this, assuming 'files' came out of fs.readdir():

async.forEach(files,

    function (file, callback) {

        // load one template file here

        // call callback(err) when fs.readFile() calls you back

    },

    function (err) {

        // now you can call templatesLoaded, since they're all done loading (or something bad happened)

    });

I should point out here that since this is code that's running at application startup, it's not strictly necessary for you to do all this with async code. Since you haven't started handling requests yet, you could just use synchronous calls instead if you wanted to.

By the way, don't confuse file system paths with URL paths. I see you've named a variable 'url' but used it for a file system path and not a URL. Keep those separate so you don't trip yourself up later.

--

Martin Cooper

[kinghokum]

Hi Martin, thanks again.

I implemented async.js and it all worked a treat.
Thanks again, I've learnt heaps about node from this simpel project and your feedback.

Cheers