Invalid anti-pattern premise

[vitaly-t]

In the following anti-pattern chapter you are going way off on a limb, only to confuse people:

The .then(success, fail) anti-pattern

The word anti-pattern in programming refers to code redundancy or compromised algorithmic efficiency.

What you refer to is simply a coding style, which got nothing to do with anti-patterns.

[benjamingr]

It's code redundancy

[bergus]

Related: https://stackoverflow.com/questions/24662289/when-is-thensuccess-fail-considered-an-antipattern-for-promises

Maybe we should relax the language a bit. It's an antipattern only in certain siturations.

[vitaly-t]

Yes, and saying:

there is almost never a reason to use .then(success, fail) in application code

almost doesn't quite cut it here, because those situation are all over the place.

That syntax changes the logic completely, and it is needed all the time. And it cannot be replaced by .catch, because that changes the logic. Let me me write an example in a moment...

So, here's my real-life example that executes a database transaction:

var cbData, cbReason, success;
obj.none('begin')
    .then(function () {
        return callback()
            .then(function (data) {
                cbData = data; // save callback data;
                success = true;
                return obj.none('commit'); // COMMIT;
            }, function (reason) {
                cbReason = reason; // save callback failure reason;
                return obj.none('rollback'); // ROLLBACK;
            })
            .then(function () {
                if (success) {
                    update(false, true, cbData); // client notification;
                    return $p.resolve(cbData); // resolve with data from the callback;
                } else {
                    update(false, false, cbReason); // client notification;
                    return $p.reject(cbReason); // reject with reason from the callback;
                }
            });
    });

So, if we replace that middle section with .catch:

obj.none('begin')
    .then(function () {
        return callback()
            .then(function (data) {
                cbData = data; // save callback data;
                success = true;
                return obj.none('commit'); // COMMIT;
            })
            .catch(function (error) {
                cbReason = error; // save callback failure reason;
                return obj.none('rollback'); // ROLLBACK;
            })
            .then(function () {
                if (success) {
                    update(false, true, cbData); // client notification;
                    return $p.resolve(cbData); // resolve with data from the callback;
                } else {
                    update(false, false, cbReason); // client notification;
                    return $p.reject(cbReason); // reject with reason from the callback;
                }
            });
    });

this changes the logic completely.

In the initial piece of code we get into the rejection section only if callback() fails, while in the modified example we will get into the catch section if either callback() fails or the commit command fails, which is an altogether different result.

[petkaantonov]

There is edit button on the top right corner

[vitaly-t]

I have updated my previous post with an example.

[petkaantonov]

How would you write it in synchronous code then?

[vitaly-t]

Not sure, haven't tried yet, it was written with promises from the beginning. Surely it would be awkward with all the nested callbacks, considering it has to rely on an asynchronous library.

[bergus]

Those global variables are horrible and seem unnecessary to me. Regardless, code like the following is perfectly reasonable:

obj.none('begin').then(function() {
    Promise.resolve()
    .then(callback)
    .then(function(data) {
        return obj.none('commit') // COMMIT;
        .then(function() {
            update(false, true, data); // client notification;
            return data;
         });
    }, function(reason) {
         return obj.none('rollback') // ROLLBACK;
         .then(function() {
            update(false, false, reason); // client notification;
            throw reason;
        });
    }); // TODO: handle commit/rollback fail
}); // TODO: handle begin fail

There is no JavaScript syntax for synchronous branching on fails. Regardless, with a pythonic try we could write

obj.none('begin');
try {
    callback();
} catch(reason) {
    obj.none('rollback'); // ROLLBACK;
    update(false, false, reason); // client notification;
    throw reason;
} else {
    obj.none('commit'); // COMMIT;
    update(false, true, data); // client notification;
    return data;
}
// TODO: handle begin/commit/rollback fail

Again, the error monad wins :-)

[vitaly-t]

Can't really do that, your code merges errors from begin and callback into one, can't tell one error from the other.

I honestly tried to refactor into using .catch, but every time the logic would change, so I see those things simply not being the same.

[bergus]

@vitaly-t Ah, right, that was the catch, you'll need that nesting here indeed. See the edit to my comment (it doesn't really change the point).
And don't get me wrong, I support your case: there is indeed no reason and no possibility to use .catch here, and your usage of then with both callbacks is not an antipattern.
It's only an antipattern for people who expect then to work like try catch, which it doesn't.

[phpnode]

@vitaly-t your example code is not really ideal for other reasons, as @bergus mentioned - it relies on out of band signalling. Here's how I'd write your example:

obj.none('begin')
.then(callback)
.catch(function (err) {
  return obj.none('rollback').then(function () {
    update(false, false, err); // client notification
    throw err;
  });
})
.then(function (data) {
  return obj.none('commit').return(data);
})
.then(function (data) {  
  update(false, true, data); // client notification
  return data;
});

[vitaly-t]

@phpnode not quite, it shows the same problem as what bergus had: within the .catch section you cannot tell the error related to begin execution from an error from the callback execution. Because of this, the code will try to execute rollback when begin fails, which it is not supposed to (it doesn't do it in my original code example).

[phpnode]

@vitaly-t that's true, this does seem like a scenario where the second argument might be an appropriate option. However I still think that most of the time it really is an anti pattern. I recently started work on a codebase where this is used a lot and it's causing them a lot of pain because it doesn't catch errors that they expect.

[vitaly-t]

We all have different experience in using promises, depending on the exact tasks at hand. My experience suggests that my example isn't that rare at all.

This is why I suggested to reconsider advocating the .then(success, error) syntax as an anti-pattern, 'cos it ain't. It can implement the kind of logic where .catch can't be used.

[benjamingr]

Do Petka's exercise of writing the code as if all operations were synchronous and the correct way will reveal itself.

[vitaly-t]

@benjamingr I wouldn't even know how to approach this, given that it is built on top of an asynchronous callback-based protocol. For the same reason I'm not sure how practical an exercise this would make.

[stefanpenner]

catch(CB).then(CB) is likely the pattern most actually intend to use. It mimics the syntactic try/catch nicely, conveys an unambiguous intent, and transposes gracefully to ES7 async/await's syntactic try/catch.

Although the then(CB,CB) induced Stockholm syndrome appears normal to some, it is unfortunate.

[benjamingr]

@vitaly-t assume the callback base protocol returns values instead or taking callbacks and that all methods return synchronous simple values.

[vitaly-t]

@benjamingr I have tried to think it that way, nothing came up yet, though I will try again. And if promises are to stand for simplicity and streamlined logic, in my example it is isn't there, or perhaps doesn't exist to begin with :)

@stefanpenner you first stated the obvious, and then ended with plain trolling, that's not helping here.

[benjamingr]

What's wrong with this @vitaly-t ?

function transact(fn){
    return obj.none("begin").then(fn).then(x => obj.none("commit").return(x), 
                                           e => obj.none("rollback").throw(SqlError(e));
}
transact(callback).tap(data => {
    update(false, true, data);
}).catch(SqlError, e => {
    update(false, false, cbReason);
    throw e;
}); // .catch(CommitError, e => /* handle error in commit */)

[stefanpenner]

@vitaly-t I'm glad you agree, if what I said was obvious that re-affirms the anti-pattern claim.

The Stockholm syndrome comment isn't trolling, it's unfortunately true.

[vitaly-t]

@benjamingr
In the following example:

return obj.none("begin").then(fn).then(x => obj.none("commit").return(x), 
                                           e => obj.none("rollback").throw(SqlError(e));

as I stated twice above, your error section can't tell whether the error was caused by begin or by the callback function, so it will try to execute rollback after a failed begin, which isn't supposed to happen.

@stefanpenner you are not really trying to follow the discussion, perhaps best not to post then ;)

[benjamingr]

@vitaly-t Oh, I didn't realize none could throw, that's super easy to fix (I thought you were talking about commit erroring vs callback).

function transact(fn){
    return obj.none("begin").catch(e => { throw TransactionStartError(e) }) 
                   .then(fn).then(x => obj.none("commit").return(x), 
                          e => isNotBeginError(e) ? reject(e) : obj.none("rollback").
                                                                    throw(SqlError(e));
}
transact(callback).tap(data => {
    update(false, true, data);
}).catch(BeginTransactionError, e => {

}).catch(SqlError, e => {
    update(false, false, cbReason);
    throw e;
}); // .catch(CommitError, e => /* handle error in commit */)

Of course we can use a coroutine here and get clearer and more verbose syntax:

const transact =  coroutine(function*(fn){
    try { 
        var begin = yield obj.none("begin");
    } catch (e) { throw new TransactionStartError(e); }
    try {
        yield fn(); // the transaction not being passed around is weird btw
    } catch(e) { return obj.none("rollback").throw(SqlError(e)); }
    yield obj.none("commit"); 
}

[vitaly-t]

Well, if we take this code from your example:

.then(x => obj.none("commit").return(x), 
                                         e => isNotBeginError(e) ? reject(e) : obj.none("rollback").throw(SqlError(e));

that's in fact the very same .then(success, error) syntax - the proclaimed anti-pattern, which is argued against here in favour of the .catch syntax, which makes the whole point of the discussion - changing my original example to such a form as to make no use of the .then(success, error) syntax, while I was arguing that in my example that can't be done.

[benjamingr]

How is that even remotely close to the code that is wrapped in a promise constructor? Note the reject here is Promise.reject and not a parameter of the promise constructor - which there is none.

I think what you were missing is that you can conditionally catch things and that in JavaScript you can rethrow errors safely.

[vitaly-t]

From what I'm reading, this code:

.then(x => obj.none("commit").return(x), 
                                         e => isNotBeginError(e) ? reject(e) : obj.none("rollback")

translates into something like:

then(function (x) {
    return obj.none("commit").return(x);
}, function (e) {
    return isNotBeginError(e) ? reject(e) : obj.none("rollback");
});

except I don't even know what isNotBeginError thing is..., plus I've never used .return either. I prefer sticking with the standard promise methods. I'm sure if we wander far enough from the standard we can find a one-line solution for everything, but that's beside the point.

[benjamingr]

Maybe an instanceof check, maybe another assertion on the error type. Whatever floats your goat.

The key here is merely that you'd rethrow the exception to signal it wasn't handled - preferably you'd wrap it in a more concrete type to signal what sort of exception it is and how someone should handle it. Half the point of exceptions (vs. using return values for errors) is the fact you can write this sort of code and propagation happens rather than writing the code you wrote above in your original example.

[vitaly-t]

Whatever floats your goat.

I don't have a goat. LOL :)

My code was in fact written to work within the Promises/A+ standard, without any specific promise library in mind, so I'm not being picky here ;)

At this point I probably will just leave a perfectly functioning code as is, because I believe that if I need to write extra code, like re-throwing exceptions, then my original code is way better, as it is simpler. And as far as the anti-patterns go, it is still very unconvincing.

[benjamingr]

My code was in fact written to work within the Promises/A+ standard, without any specific promise library in mind, so I'm not being picky here ;)

Bluebird contains a lot of sugar that makes developer lives easier - but nothing too specific in this regard - everything above can be written just fine without bluebird.

It is considerably less code, and you have a lot less room for mistakes because you don't have to manually handle cases and promises don't stay pending forever or silence errors (because control flows automatically rather than manually).

The code still ends up being clearer, less redundant and shorter, but I guess clearer is subjective. I think the update callback is pretty weird anyway :D

[benjamingr]

I'd also like to note that the pattern is there for very obvious cases of abuse, bluebird has a philosophy of doing things that cater for 95% of developers or 95% of use cases. There are always pathological examples where an anti pattern might be appropriate (also for explicit construction).

That document was written for things like:

function foo(successCb, failCb){
     somePromise.then(successCb, failCb);
}

foo(function(success) { ... }, function(failure) { ... }

Where the fact "things" happen with the callback if it throws is implicit, and it would have been a lot better to just return a promise and chain off that. This is extremely common - for example in Angular when working against $http or in jQuery working against $.ajax.

I think your (initial) attempt at the code contains (what I consider) smell, but it is a lot more debatable than the code people usually write with then(success, fail)

[vitaly-t]

Just to return to your very first post here:

It's code redundancy

Redundancy in code is defined as a repeated block of code whilst performing the exact same operation.

Using .catch versus .then(success, error) isn't repeating the same code, as the resulting logic differs. So even strictly within the terminology, there is a flaw right there in trying to label .then(success, error) as anti-pattern.

[bergus]

@stefanpenner

catch(CB).then(CB) is likely the pattern most actually intend to use.

That only works the same as .then(CB, CB) when the error handler rethrows. Which was the case in Vitaly's example, though I really don't like that style.

transposes gracefully to ES7 async/await's syntactic try/catch.

Actually try catch in async functions is superior, because you can control what parts of your code the try wraps. With .catch, it just always catches all rejections from the complete chain, unless you start nesting. Both Benjamin and me fell for that above :-)

However try catch also lacks the ability to handle normal and exception cases distinctively. @benjamingr worked around this quite well:

async function transact(fn) {
    try { 
        var begin = await obj.none("begin");
    } catch (e) {
        throw new TransactionStartError(e);
    }
    try {
        await fn(); // the transaction not being passed around is weird btw
    } catch(e) {
        return obj.none("rollback").throw(SqlError(e));
//      ^^^^^^
    }
    await obj.none("commit"); 
}

…by returning (or throwing) from within the catch. Now that is hard-to-understand control flow!

Just using .then(x => … obj.none("commit")…, e => … obj.none("rollback")…) is so much more sane imo :-)