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;
// Read the files var readFile = function (file) { fs.readFile(dir+'/'+file, 'utf8', function (err,data) { if(err) throw err; console.log("read this " + data); });
* 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.
On Fri, Nov 9, 2012 at 9:56 PM, Chris Buttery <butt...@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;
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 <but...@gmail.com<javascript:> > > 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;
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');
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 <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;
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:
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.
On Mon, Nov 12, 2012 at 4:07 AM, kinghokum <butt...@gmail.com> 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');
> };
> 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 <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;
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:
> 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 <but...@gmail.com <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'); >> };
>> 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 <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;
