3

Bear with me as a Java programmer, learning modern JavaScript on a new project. I get the concept of Promises for handling asynchronous operations but is there a reason to "promisify" code, that intensity-wise does next to nothing and doesnt contain anything you need to wait for (like db query, http request etc.)? I've run into this backend node.js code that does trivial stuff inside a promise:

const customersWithHeader; //assume this contains an array of rows loaded from CSV file
const allowedHeaderNames; //assume a string array with allowed header names (6 values)

return new Promise((resolve, reject) =>

            allowedHeaderNames.find((name, index) => name !== customersWithHeader[0][index])
                ? reject({error: 'missing-header'})
                : resolve(customersWithHeader.slice(1))

        ).then(/* Then follows code that does db queries on every row from the customer array, those are promisified by Promise.all() */)

There is nothing to wait for, just checking one tiny array against another. What is the benefit of writing such code instead of just having an if and synchronously returning the sliced array or throwing an error? To me it just seems like needlessly instantiating Promise.

Milan Štěpánek
  • 49
  • 3
  • it really depends on the original authors intent. perhaps they wanted to do some async work? – Daniel A. White Oct 31 '17 at 16:27
  • Not enough context shown to know why it was written that way. – charlietfl Oct 31 '17 at 16:29
  • Promise in this particular context of yours doesn't make sense – Ali Gajani Oct 31 '17 at 16:29
  • 1
    Yes, there is such a thing as pointless usage of a programming pattern or concept. That's as true for promises as it is for pretty much all other things. – Tomalak Oct 31 '17 at 16:31
  • 1
    I often follow an "async by default" approach because some of the data is synchronous but some is async. The code is cleaner if everything is either all async or all sync, but i can't force async things to be sync, so i force the sync stuff to be async. – bryan60 Oct 31 '17 at 16:31
  • 1
    There is also a thing called the [Explicit Promise Construction Antipattern](https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it), and this code is an example of it. – Tomalak Oct 31 '17 at 16:32
  • The only interest I could see here is to unify the error handling in promise chain to manage error consistently when the promise is consumed. – Joel Oct 31 '17 at 16:36
  • Thanks Joel, that is the only thing that makes sense to me. – Milan Štěpánek Oct 31 '17 at 16:38
  • I don't see how this code is an example of that anti pattern. This is just converting something synchronous into something asynchronous, not failure to take advantage of promise chaining? – bryan60 Oct 31 '17 at 16:39
  • 1
    @Joel normal errors thrown inside a `.then` end up getting handled with a `.catch` anyway. – TKoL Oct 31 '17 at 16:40

4 Answers4

4

One possibility would be that somebody writing that code wants to make it easily extendable in the future. Imagine that you want to quickly mock-up something, and you know it's going to be async in the future - this means the client code (the code consuming your async code) will also have to be async. If the dummy code wouldn't be async, then it would break client code once changed.

Think of it as a contract / interface type of thing.

Florin Toader
  • 337
  • 1
  • 9
1

Yes, in fact sometimes there is. One common case is that a function is expected to return a promise.

Let's say I'm using a library and one of its classes takes, as a constructor argument, a function that you implement, that essentially stores data for later. The library doesn't care how you store it, but it expects a promise back that resolves when the storage is successful. You can naturally store it using an asynchronous process, like fs.writeFile, OR if you wanted to use a synchronous storage solution (eg maybe you're in a browser and you can use localStorage), you can still return a promise, but just do synchronous stuff.

There have been many times when I've been in a situation where I needed to return a promise but didn't have any async stuff going on.

TKoL
  • 13,158
  • 3
  • 39
  • 73
  • Yes that would be a good reason, but the function that contains this code returns just a plain array. Maybe the reason as a comment higher suggests is that the author wanted to do everything inside Promises since pretty much everything else in the function (db-querying individual rows of the customer array) is done in promises. But its done in their own promises which make sense because they have to wait for mongo so its puzzling. – Milan Štěpánek Oct 31 '17 at 16:36
  • That's not really a good reason. You don't just do everything in promises if it doesn't need to be in promises. I mean the next logical step is to make an asychronous api for string concatenation, just to make everything into promises. It doesn't make sense as a reason. The only reason to make sychronous code into a promise really is if there's some reason why that synchronous code could / should be replaceable with async code. – TKoL Oct 31 '17 at 16:39
  • @MilanŠtěpánek "the function that contains this code returns just a plain array" how? You have posted it returns a promise `return new Promise((resolve, reject)...` – Yury Tarabanko Oct 31 '17 at 16:40
  • I think he means it resolves with an array (meaning it could just hypothetically return an array). – TKoL Oct 31 '17 at 16:41
  • @TKoL I doubt that `/* Then follows code that does db queries on every row from the customer array, those are promisified by Promise.all() */)` almost sure db queries are async :) – Yury Tarabanko Oct 31 '17 at 16:44
  • @MilanŠtěpánek There is a huge difference between returning an array and returning a promise that will be resolved with an array. Since I'm quite sure db queries are async you can't simply return an array. So this code is almost correct way to deal with async nature of db interactions other than it might be better to use static `Promise.resolve` and `Promise.reject` instead of explicit `new Promise` :) – Yury Tarabanko Oct 31 '17 at 16:53
  • My bad, it actually does return a promise. But still, I'd do the synchronous CSV header validation at the start and then do just the db queries inside promises. – Milan Štěpánek Oct 31 '17 at 16:55
1

Here is the alternative to the code you've displayed:

function handleError(err) {
  // do something with error
}

if (allowedHeaderNames.find((name, index) => name !== customersWithHeader[0][index])) {
    return handleError({error: 'missing-header'});
} else {
     // or some truly async thing
     return new Promise((resolve, reject) => asyncFunction()).catch(handleError);
}

Clearly it's much cleaner to just force the points in the chain to all be treated in the same async manner.

bryan60
  • 28,215
  • 4
  • 48
  • 65
1

If you work with an abstraction (interface) and expect the implementation function to return a promise then it makes sense to return Promises even for sync-code -- if you work directly with the resulting promise. For example: 

let handler = {
  handle: function(){ return Promise.resolve("something sync"); }
} 

handler.handle(ctx).then((result)=>{...

es7 async/await, however, makes this pointless because you can await a function that returns either a promise or a value; i.e. the function can be async or sync. 

async function main(){
  let handler = {
    handle: function(){ return "something"; }
  }

  let result = await handler.handle(ctx);
}
Meirion Hughes
  • 24,994
  • 12
  • 71
  • 122