Rewrite JS map into a forEach-CodePudding

How to I write the below into a foreach instead as I won't always need all resourceTypes in my urlModule. I call urlModule later and therefore need to keep the variable.

// Response coming from API

const resources = [{
  resourceType: 'brand',
  url: 'brandUrl'
}, {
  resourceType: 'product',
  url: 'productUrl'
}, {
  resourceType: 'category',
  url: 'categoryUrl'
}, {
  resourceType: 'document',
  url: 'documentUrl'
}];

Change .map into a forEach to avoid the "Expected to return a value at the end of arrow function" warning?

      const urlModule = resources.map((resource) => {
        const { resourceType } = resource

        if (resourceType === 'brand') {
          return {
            url: `/${resource.url}`,
            changefreq: 'weekly',
            priority: 0.9,
          }
        }
        if (resourceType === 'product') {
          return {
            url: `/${resource.url}`,
            changefreq: 'monthly',
            priority: 0.8,
          }
        }
        if (resourceType === 'document') {
          return {
            url: `/${resource.url}`,
            changefreq: 'weekly',
            priority: 0.6,
          }
        }
      })
return [
        {
          url: '/',
          changefreq: 'daily',
          priority: 1,
        },
        {
          url: '/account',
          changefreq: 'daily',
          priority: 1,
        },
        ...urlModule,
      ]
    },

CodePudding user response：

You have to create new variable outside of the loop then.

let urlModule = [];
resources.forEach((resource) => {
    const { resourceType } = resource

    if (resourceType === 'brand') {
      urlModule.push({
        url: `/${resource.url}`,
        changefreq: 'weekly',
        priority: 0.9,
      })
    }
    if (resourceType === 'product') {
      urlModule.push({
        url: `/${resource.url}`,
        changefreq: 'monthly',
        priority: 0.8,
      })
    }
    if (resourceType === 'document') {
      urlModule.push({
        url: `/${resource.url}`,
        changefreq: 'weekly',
        priority: 0.6,
      })
    }
  });
  
  console.log(urlModule);

CodePudding user response：

const resources = [{
  resourceType: 'brand',
  url: 'brandUrl'
}, {
  resourceType: 'product',
  url: 'productUrl'
}, {
  resourceType: 'category',
  url: 'categoryUrl'
}, {
  resourceType: 'document',
  url: 'documentUrl'
}];

const urlModule = [];

resources.forEach((resource) => {
  const { resourceType } = resource

  if (resourceType === 'brand') {
    return urlModule.push({
      url: `/${resource.url}`,
      changefreq: 'weekly',
      priority: 0.9,
    });
  }
  if (resourceType === 'product') {
    return urlModule.push({
      url: `/${resource.url}`,
      changefreq: 'monthly',
      priority: 0.8,
    });
  }
  if (resourceType === 'document') {
    return urlModule.push({
      url: `/${resource.url}`,
      changefreq: 'weekly',
      priority: 0.6,
    });
  }
});
  
console.log(urlModule);

CodePudding user response：

You must have to create new array outside of the forEach loop.

const urlModule = [];
resources.forEach(resource => {
    const {resourceType} = resource;

    if (resourceType === 'brand')
        urlModule.push({url: `/${resource.url}`, changefreq: 'weekly', priority: 0.9})

    if (resourceType === 'product')
        urlModule.push({url: `/${resource.url}`, changefreq: 'monthly', priority: 0.8})

    if (resourceType === 'document')
        urlModule.push({url: `/${resource.url}`, changefreq: 'weekly', priority: 0.6})
})

CodePudding user response：

One approach would be to use a flatMap here.

It works like a map, but you can return zero or more elements instead of always having to return exactly 1.

  const urlModule = resources.flatMap((resource) => {
    const { resourceType } = resource

    if (resourceType === 'brand') {
      return [{
        url: `/${resource.url}`,
        changefreq: 'weekly',
        priority: 0.9,
      }]
    }
    if (resourceType === 'product') {
      return [{
        url: `/${resource.url}`,
        changefreq: 'monthly',
        priority: 0.8,
      }]
    }
    if (resourceType === 'document') {
      return [{
        url: `/${resource.url}`,
        changefreq: 'weekly',
        priority: 0.6,
      }]
    }

    return []
  });
  
  console.log(urlModule);

You could also use a filter() followed by a map().

There are plenty of ways to approach this. It's up to you which you prefer.

CodePudding user response：

There are several ways to approach this, and forEach is probably the least elegant.

Regarding "Expected to return a value at the end of arrow function", this is actually useful, I'd personally want to avoid functions that just fall through without a return in some cases, as its less clear that's the intention. This warning is telling you that you have opaque logic, but refactoring too forEach just to avoid this has its own issues.

The mapper is reusable, so extract it

First of all we can extract the map callback as we can reuse the logic regardless of how we organise the rest of the code.

Refactor with `forEach`

To use forEach, we need to establish a variable outside of the scope of the mapper callback into which we will insert the expected output. Personally I try to avoid this as the declaration often gets separated from the forEach call and can lead to confusion as to where the referenced variable comes from (could be global, not use outer scope).

Refactor to `map` then `filter`

In this variant, we retain the existing map. This results in some of the array entries being undefined, but these can be easily filtered out. In my example I'm using a double logical not to convert the value to a boolean, as expected by filter. Any value that is undefined will return false and be removed.

Refactor to `reduce`

Reduce is similar to the forEach variant but has the advantage that it encapsulates the receiving variable into the reduce call, so our callback doesn't have an external dependency. Here I've reused the existing mapper into the reduce callback, but you could encorporate the logic into the callback if this is the only place that you use it.

Bonus - `switch` instead of `if`

Pesonally I'd refactor the mapper to use switch. As written, multiple conditionals are evaluated even after a match, which could lead to errors when future modifications are made. The computational overhead is probably negligible. I find that switch indicates to the reader that 'one and only one of these applies'

const resources = [{
    resourceType: 'brand',
    url: 'brandUrl'
}, {
    resourceType: 'product',
    url: 'productUrl'
}, {
    resourceType: 'category',
    url: 'categoryUrl'
}, {
    resourceType: 'document',
    url: 'documentUrl'
}];

// ------------------------------ Our mapping callback
const mapper = (resource) => {
    const { resourceType } = resource

    if (resourceType === 'brand') {
        return {
            url: `/${resource.url}`,
            changefreq: 'weekly',
            priority: 0.9,
        }
    }
    if (resourceType === 'product') {
        return {
            url: `/${resource.url}`,
            changefreq: 'monthly',
            priority: 0.8,
        }
    }
    if (resourceType === 'document') {
        return {
            url: `/${resource.url}`,
            changefreq: 'weekly',
            priority: 0.6,
        }
    }
    return undefined;
}

// ------------------------------ forEach instead of map
const foreach = []
resources.forEach(resource => {
    const item = mapper(resource)
    if (item !== undefined) {
        foreach.push(item)
    }
})
console.log('foreach', foreach)

// ------------------------------ Map then filter
const mapFilter = resources
    .map(mapper)
    .filter(item => !!item)

console.log('mapfilter', mapFilter);

// ------------------------------ Reduce
const reduce = resources
    .reduce((acc, cur) => {
        const item = mapper(cur)
        if (item !== undefined) {
            acc.push(item)
        }
        return acc
    }, [])

console.log('reduce', reduce)

CodePudding user response：

From my above comment ...

"Changing from a correct method to a less suitable one just in order to avoid a linter warning doesn't prevent the code smell. One just changes the odor. Stay with map and change the implementation according to a single return value. "

If one extracts the if clause based data and provides them as object based lookup, an arrow function based mapper, with the help of some spread syntax, could be boiled down to something as simple as e.g. ...

const createUrlItem = ({ resourceType, url }) => ({
  url,
  ...(urlItemLookup[resourceType] ?? {}),
});

const resources = [{
  resourceType: 'brand',
  url: 'brandUrl',
}, {
  resourceType: 'product',
  url: 'productUrl',
}, {
  resourceType: 'category',
  url: 'categoryUrl',
}, {
  resourceType: 'document',
  url: 'documentUrl',
}];

const urlItemLookup = {
  brand: {
    changefreq: 'weekly',
    priority: 0.9,
  },
  product: {
    changefreq: 'monthly',
    priority: 0.8,
  },
  document: {
    changefreq: 'weekly',
    priority: 0.6,
  },
};
const urlModule = resources
  .map(({ resourceType, url }) => ({
    url,
    ...(urlItemLookup[resourceType] ?? {}),
  }));
  
console.log({ urlModule, resources });

.as-console-wrapper { min-height: 100%!important; top: 0; }

The mapper is reusable, so extract it

Refactor with forEach

Refactor to map then filter

Refactor to reduce

Bonus - switch instead of if

Refactor with `forEach`

Refactor to `map` then `filter`

Refactor to `reduce`

Bonus - `switch` instead of `if`