CODEWITHSUNDEEP
javascriptnode.jsforeach
Quasistar
Quasistar

Reputation: 23

Reusing a variable into a loop - NodeJS

I am kinda new to Node.JS and am trying to write a "synchronous" loop in Node.JS, which in theory should do as follow:

  1. Loop over an object array, in order
  2. Apply a function on each object (actually creates the object in the DB and returns its unique id)
  3. After the first object has been created, use its id as parent_id for all other objects.

Problem is, I am lost between async/sync nature of callbacks and functions. E.g. ids.push won't give the expected result.

Edit: additionally, I am currently bound to Node 6.9 because of project constraints.

Code is tentatively as follows:

    function processAll( objectArray, callback ) {
        let ids = [];

        // Loop on all data
        objectArray.forEach( ( obj ) => {
            // From the second iteration on, 
            // objects are children of first obj 
            if( ids.length ) {
                obj.parent_id = ids[0];
            }
            someLib.doSomething( obj, ( err, result ) => {
                if( err ) {
                    return callback( err );
                }
                // This won't work, of course
                ids.push( result );
            });
        });
        return callback( null, ids );
    }

Upvotes: 2

Views: 865

Answers (6)

user1366860
user1366860

Reputation:

One more response with a slightly different approach.

As you mention correctly and as some of the previous answers also comment, Array.prototype.forEach is not asynchronous aware; instead of waiting for an item to be ready before jumping into the next iteration, it just calls all of the items as soon as possible.

Using promises is a good approach but it needs you to know how to work with promises and how to convert an old style callback function into a promise one. This is already present in another answer so I won't explain it.

I can see your code is not using promises, so giving a promise approach is more confusing than helpful; instead I'll give you an option with a library that has been around for years and is battle tested: Async. It allows you to easily perform async operations very easily without breaking your head on how to handle these cases.

You can actually use this code snippet to see the results in a terminal after installing async with npm install async. Also, I am mocking your someLib.doSomething assuming an async operation.

// Saved this code to a file named sync-loop.js for tests.
const async = require('async');
const items = [{ name: 'foo' }, { name: 'bar' }, { name: 'baz' }];

const someLib = {
  doSomething(obj, callback) {
    console.log('someLib.doSomething', obj);

    const randomValue = parseInt(Math.random() * 1000, 10);

    setTimeout(() => {
      callback(null, randomValue);
    }, randomValue);
  },
};

function processAll(objectArray, processAllCallback) {
  async.reduce(objectArray, [], (ids, item, reduceCallback) => {
    if (ids[0]) {
      item.parent_id = ids[0];
    }

    someLib.doSomething(item, (err, result) => {
      if (err) {
        reduceCallback(err);
      }

      ids.push(result);
      reduceCallback(null, ids);
    });
  },
    processAllCallback
  );
}

processAll(items, (err, ids) => console.log(ids));

Running this gives me a similar response to this:

$ node sync-loop.js
someLib.doSomething { name: 'foo' }
someLib.doSomething { name: 'bar', parent_id: 145 }
someLib.doSomething { name: 'baz', parent_id: 145 }
[ 145, 179, 816 ]

Upvotes: 1

Konstantin Rybakov
Konstantin Rybakov

Reputation: 385

You may consider using following construct instead of objectArray.forEach() as it will avoid using a callback:

  for(let obj of objectArray){ 
    //..do stuff..
   }

Also, if your someLib.doSomething is also asyncronous and returns Promise - you may consider make your entire method asyncrhonous and just await for result. This way you will be sure your iterations will go one after another.

async function processAll( objectArray, callback ) {
    let ids = [];

    for(let obj of objectArray){
        if( ids.length ) {
            obj.parent_id = ids[0];
        }

        let resultDoSomething = await someLib.doSomething( obj );

        if( resultDoSomething.err ) {
            return callback( resultDoSomething.err );
        }
            ids.push( resultDoSomething.result );
        }

        return callback(null, ids);  
}

Upvotes: 0

Aluan Haddad
Aluan Haddad

Reputation: 31833

Use async/await and promisify

const util = require('util');

async function processAll([parent, ...children]) {
    let childIds = [];
    const doSomething = util.promisify(someLib.doSomething.bind(someLib));

    const parentId = await doSomething(parent);
    for (const child of children) {
        child parent_id = parentId;
        const childId = await doSomething(child);
         childIds.push(childId);
    } 
    return [parentId, ...childIds];
}

Notice that the code is simplified to the extent where the comments have become superfluous. Notice the removal of the callback - we can simply return our results and any error raised by the promisified doSomething will be automatically propagated to the caller.

Speaking of which, the calling code will change from

processAll(objects, (err, ids) => {
    if (err) {
      console.error(err);
    } else { 
     console.log(ids);
    }
 });

To 

 processAll(objects)
     .then(ids => { 
         console.log(ids);
     })
     .catch(err => { 
         console.error(err);
     });

Upvotes: 0

Spitzbueb
Spitzbueb

Reputation: 5891

Your code works, if someLib.doSomething() is synchronous. There might be a problem when it's asynchronous as it may process the second element before the first one is done.

If you wan't to make sure the first item is processed before you insert the other ones you have to add it separately and add the others in it's callback method. Pseudo code would look a bit like this:

someLib.doSomething(firstElem, processOtherElems);

function processOtherElems() {
  object.forEach(obj => someLib.doSomething(obj, callbackForEachObjectCreated);
}

Example of synchronous code that works:

// define a sample of object we want to insert into the database
let objectArray = [
  {name: "One"},
  {name: "Two"},
  {name: "Three"}
];
// define a callback method that logs the ids, once the insert of ALL object is completed
let callback = (x, y) => console.log(y);

// mock the library
let someLib = {
  doSomething: function(object, callback){
    let newId = Math.round(Math.random() * 10000);
    callback(null, newId);
  }
};

// our synchronous process function
function processAll( objectArray, callback ) {
  let ids = [];

  // Loop on all data
  objectArray.forEach( obj => {
      // From the second iteration on, 
      // objects are children of first obj 
      if( ids.length ) {
          obj.parent_id = ids[0];
      }
      someLib.doSomething( obj, ( err, result ) => {
          if( err ) {
              return callback( err );
          }
          // This WORKS if someLib.doSomething is synchronous
          // however, if it's truly asynchronous, it might process the second item before the first one is done 
          ids.push( result );
      });
  });
  return callback( null, ids );
}

processAll(objectArray, callback);
console.log(objectArray);

Upvotes: 0

sanniboy
sanniboy

Reputation: 1

the forEach loop is probably already finished before your database callback is made, meaning you will not get an parent id to set as a property for the child objects. To ensure that, you must start looping the rest of your array after the database callback. there are multiple approaches to achieve this, below is a quick and dirty way.

function processAll( objectArray, callback ) {
    let ids = [];
    let parent = objectArray[0]; // the first item in the array
        someLib.doSomething( parent, ( err, result ) => {
            if( err ) {
                return callback( err );
            }
            // now you have an ID from your callback and can be pushed into ids variable
            ids.push( result );
            // start an oldskool for loop from index 1 instead of forEach
            for(let i=1; i < objectArray.length; i++){
                objectArray[i].parent_id = ids[0]
            }

            return callback( null, ids );
        });    
}

hope this helps

Upvotes: 0

Zawiszor
Zawiszor

Reputation: 538

I think I know whats the problem. I'm assuming that function someLib.doSomething is asynchronous which means that JS won't wait for it to finish before proceeding to the next line. This means that your function will return before the code had its time to get all the values from the DB. Look into Promises they are extremely helpful to clean up asynchronous code. If you provide more source code I might be able to help you refactor it.

Upvotes: 0

Related Questions