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 On Mon, Nov 12, 2012 at 4:07 AM, kinghokum <[email protected]> wrote: > 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(); > > > > > > > > On Monday, November 12, 2012 8:23:16 AM UTC+11, kinghokum wrote: >> >> Thanks Martin, I appreciate it. >> I'll look into how to implement your suggestions. >> >> On Monday, November 12, 2012 5:42:39 AM UTC+11, Martin Cooper wrote: >>> >>> 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 <[email protected]> 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<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 [email protected] >>>> To unsubscribe from this group, send email to >>>> [email protected] >>>> For more options, visit this group at >>>> http://groups.google.com/**group/nodejs?hl=en?hl=en<http://groups.google.com/group/nodejs?hl=en?hl=en> >>>> >>> >>> -- > 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 [email protected] > To unsubscribe from this group, send email to > [email protected] > For more options, visit this group at > http://groups.google.com/group/nodejs?hl=en?hl=en > -- 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 [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/nodejs?hl=en?hl=en
