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 On Wednesday, November 14, 2012 3:43:12 AM UTC+11, Martin Cooper wrote: > > 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] <javascript:> > > 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]<javascript:> >> To unsubscribe from this group, send email to >> [email protected] <javascript:> >> 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
