CODEWITHSUNDEEP
javascriptnode.jsasynchronouspromise
Nurudeen Amedu
Nurudeen Amedu

Reputation: 49

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

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

Upvotes: 0

Views: 112

Answers (4)

ktilcu
ktilcu

Reputation: 3128

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.

  1. 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.

  2. 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)
    )
  );
}

Upvotes: 1

Thomas
Thomas

Reputation: 12637

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)

Upvotes: 1

Andrea Franchini
Andrea Franchini

Reputation: 576

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
}

Upvotes: 1

Patrick Roberts
Patrick Roberts

Reputation: 51846

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.

Upvotes: 1

Related Questions