Hacker News new | ask | show | jobs
by jQueryIsAwesome 4826 days ago
All the code and such a big abstraction for the first example when it could have done like this:

    var result = [];
    paths.forEach(function (i, file){
        fs.stat(file, function (err, data){
            result.push(data);
            if (i === 0) {
                // Use stat size
            }
            if (result.length === paths.length) {
                // Use the stats
            }
        });
    });
Fairly understandable, more efficient and without introducing logic patterns foreign to many. It also meet his requirements (It is parallel and we only hit every file once)
4 comments

To be fair, you haven't handled the complete case. What if one of the items fails? You need to handle the error, but only if it's the first error, and make sure to tell all later-called callbacks that they're too late and we have already failed. Except, if we got an error on an early callback but the 0th item comes back later, we need to do whatever we were going to do with that one piece of data.

    var result = [];
    var hasFailed = false;
    paths.forEach(function (i, file){
        fs.stat(file, function (err, data){
            // if previous callback failed, give up.
            // unless this is the first item, which we still need
            if(hasFailed && i !== 0) return;
            if(err) {
                hasFailed = true;
                // do something with the error 
                if (i === 0) // do something special for error on first item.
                return;
            }
            result.push(data);
            if (i === 0) {
                // Use stat size
                // remember we might have already failed, in which case don't add the first item to the general result
                if(hasFailed) return;
            }
            if (result.length === paths.length) {
                // Use the stats
            }
        });
    });
That's almost certainly still not close to right. Which just illustrates the basic problem: without either promises or something like async.js, you're reimplementing control flow by yourself. You can easily start with perfectly-nice-looking code that balloons to be incomprehensible as soon as you start caring about error cases. And where two statements, perhaps dozens of lines apart, are preserving some invariant that is not obvious to someone editing your code in the future. Even yourself.
The example I'm talking about is the one where he doesn't handle errors either, this one: http://pastebin.com/98CarwzU.

And your example is disingenuous too, why do we need to add so much logic instead of collecting the errors if you want to handle them anyway?

    result.push(err || data);
So you don't need to copy around a variable called "hasFail", one line can be enough

    var failed = results.every(Buffer.isBuffer);
> without introducing logic patterns foreign to many

Consider your use of the forEach() abstraction when you could have used a for() loop just as easily. (That said, I agree the article in general could do a better job of describing the "other" side)

Nope, the shared scope would obligate me to create a function with binded params for each iteration so "i" is not the last value (e.g. path.length) in every call. Examples: http://stackoverflow.com/questions/1451009/javascript-infamo...
Exactly, one of the advantages of using a good abstraction like forEach() is that it prevents you from making that kind of mistake. On the other hand, it can be more costly in cases where you don't need the closure. All abstractions by definition have some tradeoffs, some visible or hidden complexity, and some extra knowledge. Promises are no different.
I see no benefits in promises, zero, none.
The real problem, design-wise, is that fs.stat operates on a single file at a time. Sometimes you only want info on one file, sure, but in many common use cases, you want info on a bunch of files - perhaps even the contents of an entire directory, or a directory tree. Worse still, stat might be a syscall! Woo, syscall per file.
... and what does that haves to do with promises?

And in such case you would only do this once outside the listeners of http/or-whatever connections so it would be done just once regardless of the number of concurrent activity.

The point is that a properly designed API wouldn't require any amount of scaffolding. You'd go:

    fs.statMany(filenames, function (stats) { ... });
or:

    var statsPromise = fs.statMany(filenames);
And then in either case, you'd just use a for-loop or forEach or whatever your preference on the result. No thinking about how to preserve complex invariants or whatever is necessary.

Hell, with ES6 generator-y expressions you could make it even more succinct, something like:

    var result;
    fs.statMany(filenames, function (stats) { result = [... for x in stats]; });
No push nonsense, no nested if statements, no need to explicitly invoke async.parallel or whatever. Just clarity.
Sorry, I don't see no clarity there. And how would that fix the fact that stat works in individual files?, and more importantly, how does a function like that handles error? Individual level, group level? It is confusing.
You've missed so many edge cases it's not even funny. Error handling? Zero length "paths" array? These are exactly the sort of things that using promises helps avoid.

Why don't we all just go back to manually pushing/popping arguments on the stack like in assembly language while we're at it?