How can i rewrite these JavaScript promises to be less complicated?

Question

I wrote this, but it looks rough, its for node js so all values need to be gotten in order

import tripfind from '../model/tripfind';
import addbook from '../model/addbook';
import bookfind from '../model/bookfind';

function addBook(info) {

  return new Promise((resolve, reject) => {
    let rowcount = -1;
    bookfind(0, -1, -1).then((result) => {
      if (result) {
        rowcount = result.rowCount;
      } else {
        rowcount = 0;
      }
      tripfind(info.trip_id, 0).then((xresult) => {
        let val = '';
        if (xresult === 'false') {
          val = 'trip is not valid';
          resolve(val);
        } else {
          addbook(info, rowcount).then((value) => {
            if (value === 'invalid id') {
              resolve('invalid id');
            }
            resolve(value);
          }).catch((err) => {
            if (err) {
              reject(err);
            }
          });
        }
      }).catch((error) => {
        reject(error);
      });
    }).catch((err) => {
      if (err) {
        reject(err);
      }
    });
  });
}

export default addBook;

thats the code above, the code also gets exported and is handled as a promise function, please help if you can

Just return the values, and delete all of the catch callbacks. https://stackoverflow.com/q/23803743/34397 — SLaks, Jul 17 '19 at 15:31
You're missing some `return`s... can be written more crealy in async await approach.. i'll write an exmple — Andrea Franchini, Jul 17 '19 at 15:31
[codereview.se] is the more appropriate place to ask about improving code style. — Barmar, Jul 17 '19 at 15:41

Andrea Franchini · Answer 1 · 2019-07-17T15:44:35.517

You're missing some returns of promises, and if you like this way of writing you should indent then() and catch() in the same way. Also doing this

  .catch((err) => {
    if (err) {
      reject(err);
    }
  });

is useless, caus you catch an error to reject it. Rewritten in promise pattern (and a bit of optimization):

function addBook(info) {

    return bookfind(0, -1, -1)
      .then((result) => {
        rowcount = result ? result.rowCount : 0

        return tripfind(info.trip_id, 0)
          .then((xresult) => {
            if (xresult === 'false') {
              return 'trip is not valid'
            }

            return addbook(info, rowcount)
          })
      })
}

Anyway is more clear with the async/await pattern:

async function addBook(info) {

  let result = await bookfind(0, -1, -1)

  rowcount = result ? result.rowCount : 0

  let xresult = await tripfind(info.trip_id, 0)
  if (xresult === 'false')
    return 'trip is not valid'

  let value = await addbook(info, rowcount)
  return value
}

score 1 · Answer 2 · answered Jul 17 '19 at 15:41

Without using async / await, this example is functionally equivalent to what you had before:

function addBook (info) {
  return bookfind(0, -1, -1).then(result => {
    const { rowCount } = result || { rowCount: 0 };

    return tripfind(info.trip_id, 0).then(trip =>
      trip === 'false'
      ? 'trip is not valid'
      : addbook(info, rowCount)
    );
  });
}

Sections like

.then((value) => {
  if (value === 'invalid id') {
    resolve('invalid id');
  }
  resolve(value);
})

.catch((err) => {
  if (err) {
    reject(err);
  }
});

.catch((error) => {
  reject(error);
});

are completely superfluous, so removing them doesn't affect the logic at all. After removing those, and unwrapping your Promise constructor antipattern, you can see the logic becomes a lot more simplified.

score 1 · Answer 3 · answered Jul 17 '19 at 15:46

like this?

function addBook(info) {
  return bookfind(0, -1, -1).then((result) => tripfind(info.trip_id, 0)
    .then((xresult) => (xresult === 'false') ? 
      'trip is not valid' : 
      addbook(info, result ? result.rowCount : 0)
    )
  );
}

or with async/await

async function addBook(info) {
  const result = await bookfind(0, -1, -1);

  const xresult = await tripfind(info.trip_id, 0)

  return xresult === "false"? 
    'trip is not valid' : 
    addbook(info, result ? result.rowCount : 0);
}

Avoid the Promise/Deferred antipattern: What is the explicit promise construction antipattern and how do I avoid it?

and remove pointless/useless code, like this function for example:

(value) => {
  if (value === 'invalid id') {
    resolve('invalid id');
  }
  resolve(value);
}

which always resolves to value. that's like return value === 1? 1: value

Or all the (error) => { reject(error); } just to "forward" the error to the returned promise.

or this construct

let rowcount = -1;

//....

  if (result) {
    rowcount = result.rowCount;
  } else {
    rowcount = 0;
  }

//....

doSomethingWith(rowcount)

which can be summed up as doSomethingWith(result ? result.rowCount : 0)

I particularly like how you moved the `result ? result.rowCount : 0` expression to the parameter list. — Patrick Roberts, Jul 17 '19 at 15:50

ktilcu · Answer 4 · 2019-07-17T19:26:00.653

A lot of people might tell you that async/await will solve this better. I think the problem is more about how the promises are used. If you have access to change the functions you are calling here's what I'd recommend.

Have functions return things of the same shape - e.g., the call to bookfind can return a rejected promise or a resolved promise that wraps undefined or {..., :rowcount:12}. If we remove the undefined result and return a default {..., rowcount:0 } it would simplify the calling functions substantially and contain logic a bit better.
Remove the superfluous function calls (.catchs and the addbook .then bit)

Here's what those functions could look like (not knowing your setup at all)

function tripfind(id, num) {
  return new Promise(function(resolve, reject) {
    doSomething(function(err, results) {
      if (err) return reject(err);
      if (results === "false") return reject(new Error("trip is not valid"));
      return resolve(results);
    });
  });
}

function bookfind(id, start, end /*or whatever*/) {
  return new Promise(function(resolve, reject) {
    findBooks(function(err, results) {
      if (err) return reject(err);
      // handle odd cases too
      if (!results) return resolve({ rowcount: 0 });
      return resolve(results);
    });
  });
}

and the usages

bookfind(0, -1, -1).then(results =>{}).catch(err=>{})
tripfind(id, 0).then(results =>{}).catch(err=>{})

Using Promises this way, we don't have to check the value of results because if it wasn't there, the promise shouldn't resolve. Likewise, the shape of the thing coming out of the promise should always be the same. (I call it a smell when a function returns String|Object|Undefined, it's possible but I'd look hard at it first)

Using this pattern and removing calls that don't do anything (all the .catch calls), we can simplify the code down to:

function addBook(info) {
  return bookfind(0, -1, -1).then(({ rowcount }) =>
    tripfind(info.trip_id, 0).then(() =>
      addbook(info, rowcount)
    )
  );
}

Based on your philosophy, shouldn't the inner-most return value be `result === "false" ? Promise.reject(new Error("trip is not valid")) : addbook(info, rowcount)`? — Patrick Roberts, Jul 17 '19 at 19:18
I was trying to balance how much to rewrite with making a point. Your answer had the straightforward rewrite so i was trying to provide something else. In this case, I'd put the false checking in the `tripfind` function. — ktilcu, Jul 17 '19 at 19:23
That's a fair point. I wasn't attempting to debunk your answer, just making sure I didn't misunderstand anything. I appreciate you integrating my feedback into your answer though. — Patrick Roberts, Jul 17 '19 at 19:26
Thanks for pointing it out! I did in fact update the answer. ;) — ktilcu, Jul 17 '19 at 19:27

How can i rewrite these JavaScript promises to be less complicated?

4 Answers4