Using for loops create new object with other objects with matching id's

Question

I'm not sure if this is the best way to write this code..

object.array.forEach((item) => {
  object.otherarray.array.forEach((asset) => {
    if(item.fields.image.sys.id === asset.sys.id) {
      item.fields.image['fields'] = asset.fields;
    }
  })
});

any help would be appreciated!

Answer 1

In this loop object.otherarray.array you use id property to to ensure to have necessary match. So I assume this item.fields.image.sys.id === asset.sys.id always should have only one match. And it resulted in the following code:

object.array.forEach((item) => {
    item.fileds.image.fields = object.otherarray.array.filter(
        (asset) => item.fields.image.sys.id === asset.sys.id
    )[0].fields;
});

Some explanation:

object.otherarray.array.filter(
        (asset) => item.fields.image.sys.id === asset.sys.id
    )[0];

is an equivalent of your item.fields.image.sys.id === asset.sys.id. In other words, firstly we find necessary asset. Then when we have it we can use fields property to assign it to item.fileds.image.fields.

I also have another variant of my remake:

object.array.forEach((item) => {
    const imageId = item.fields.image.sys.id;
    item.fileds.image.fields = object.otherarray.array
        .filter((asset) => imageId === asset.sys.id)
        [0].fields;
});

While it is more verbose version, it is more clear and structive so I would recommend to use this one.

I am open to your comments as I suppose I could not understand your correctly.

Edit: as @MarcRohloff said, you can use find instead of filter. But note, that `find has poor support in browsers:

object.array.forEach((item) => {
    const imageId = item.fields.image.sys.id;
    item.fileds.image.fields = object.otherarray.array
        .find((asset) => imageId === asset.sys.id)
        .fields;
});

Answer 2

Note

Code review is for working code, not a generic "How I might write some functionality?" which I feel your example is. I must take the code at face value and that it is in accordance to the site rules.

General points

• The array otherarray is poorly named and should be otherArray.
• Additionally the names array and otherarray are extremely poor and give no clue as to what they hold. Never name variable to their type, name them for what they represent.
• Why use the more complicated bracket notation for accessing fields there is no need. item.fields.image['fields'] can be item.fields.image.fields
• Single parameters for Arrow functions do not need to be delimited with (...) thus array.forEach((item) => { can be array.forEach(item => {
• Reduce complexity by assigning long property paths in the outer loop (see example for item.fields.image.sys.id)
• This is inline code and not a good example of quality code. Always encapsulate functionality in a function.
• As a function you can extract the two arrays via parameter destructuring (see examples).

• It may or may not be a requirement that you copy the reference to fields to the first array, meaning that any changes to the original will appear in the copied reference.

  As your intent is unclear I will just point out that you can make a shallow copy using item.fields.image.fields = {...asset.fields} or if the left side exists you may want to just copy over existing fields and add new ones with Object.assign(item.fields.image.fields,asset.fields);

Complexity

Without the data I must make an assumption that there is only a one to one match for otherarray and array on items sys.id which makes the inner loop inefficient because you keep looping after you have found the only match. If this is not the case then you would be assigning the last match in which case you would use a for ; ; or while loop to iterate backward to find the last match.

Reducing inner iterations via Array.some or for...of

You could use array.some to find the first match which can greatly reduce the number of iterations needed. Or more efficiently you can use a for...of loop and break once a match is found. Both solutions below.

Example using Array.some

function assignFieldsToMatchId({array, otherArray}) { 
    array.forEach(item => {
        const id = item.fields.image.sys.id;
        otherArray.array.some(asset => {
            if (id === asset.sys.id) {
                item.fields.image.fields = asset.fields;
                return true; // breaks out of the inner loop.
            }
            return false;
        })
    });
}
assignFieldsToMatchId(object);  /// You need to ensure otherarray is named otherArray

Example using inner for...of

function assignFieldsToMatchId({array, otherArray}) { 
    array.forEach(item => {
        const id = item.fields.image.sys.id;
        for (const asset of otherArray.array) {
            if (id === asset.sys.id) {
                item.fields.image.fields = asset.fields;
                break;
            }
        }
    });
}
assignFieldsToMatchId(object); /// You need to ensure otherarray is named otherArray

Example using only for...of

Using for...of loops are always slightly more efficient than the Array iterators like forEach

function assignFieldsToMatchId({array, otherArray}) { 
    for (const item of array) {
        const id = item.fields.image.sys.id;
        for (const asset of otherArray.array) {
            if (id === asset.sys.id) {
                item.fields.image.fields = asset.fields;
                break;
            }
        }
    }
}
assignFieldsToMatchId(object); /// You need to ensure otherarray is named otherArray