2
\$\begingroup\$

On my app I'm checking the equality between an objects array property and a nested object property - based on the results I return a new objects array.

The items objects array:

[{name: 'test', value: 1, category: 'a'},{name: 'test2', value: 2, category: 'b'},{name: 'test3', value: 3, category: 'c'}]

The itemList object:

{testnum1: {name: 'test123', category: 'a'}, testnum2: {name: 'test234', category: 'b'}

I need to check the equality between the category properties and return the following objects array:

[{name: 'test', itemCategory: 'testnum1'},{name: 'test2', itemCategory: 'testnum2'}]

This is what I did, any idea on how to make it more readable?

const getCategorizedItems = items => {
  const itemListCategories = Object.keys(itemList)
  const categorizedItems= items.map(item=> {
    const itemCategory = itemListCategories.find(category =>
      item.category === itemList[category].category 
    )
    if (itemCategory ) return {name: item.name, category: itemCategory }
  })
  .filter(f => f)
  return categorizedItems
}
\$\endgroup\$
1
  • 2
    \$\begingroup\$ Welcome to CodeReview@SE. Please add to your question how the code presented is going to be used: What is it good for? (Re)Visit How do I ask a Good Question? \$\endgroup\$
    – greybeard
    Commented Jan 12, 2021 at 10:09

3 Answers 3

4
\$\begingroup\$

Short review;

  • Unless you are writing inline functions, you should stick to using function
  • Using semicolons is preferred, it takes skill and knowledge to avoid the pitfalls of not using semicolons
  • You assign in 1 statement the value of categorizedItems and return it in the next, you might as well return immediately
  • In essence you do a find within a map, not too fast, I would create a lookup structure before the map
  • itemList seems like a terrible name, I would call it categories

I would counter-propose the following;

const items = [{name: 'test', value: 1, category: 'a'}, {name: 'test2', value: 2, category: 'b'}, {name: 'test3', value: 3, category: 'c'}];
const categories = {testnum1: {name: 'test123', category: 'a'}, testnum2: {name: 'test234', category: 'b'}};
                    
function getCategorizedItems(items, categories){
  //Desired output; [{name: 'test', itemCategory: 'testnum1'},{name: 'test2', itemCategory: 'testnum2'}]
  const categoriesMap = Object.fromEntries(Object.entries(categories).map((entry) => [entry[1].category, entry[0]]));
  return items.map(item => ({name: item.name, itemCategory:categoriesMap[item.category]})).filter(item => item.itemCategory);
}

console.log(getCategorizedItems(items, categories));

\$\endgroup\$
2
\$\begingroup\$

I would start by extracting the relevant data from itemList and create a lookup object that maps the category to the relevant key/itemCategory.

const itemCategories = new Map(
  Object.entries(itemList)
    .map(([itemCategory, {category}]) => [category, itemCategory])
);

With the above structure you can filter the items and only keep the items that have a category property that is present within the lookup object. After filtering the array you can map the data into the desired result.

items
  .filter(item => itemCategories.has(item.category))
  .map(item => ({
    name: item.name,
    itemCategory: itemCategories.get(name.category)
  }));

const items = [{name: 'test', value: 1, category: 'a'}, {name: 'test2', value: 2, category: 'b'}, {name: 'test3', value: 3, category: 'c'}];
const itemList = {testnum1: {name: 'test123', category: 'a'}, testnum2: {name: 'test234', category: 'b'}};

const itemCategories = new Map(
  Object.entries(itemList)
    .map(([itemCategory, {category}]) => [category, itemCategory])
);

function getCategorizedItems(items) {
  return items
    .filter(item => itemCategories.has(item.category))
    .map(item => ({
      name: item.name,
      itemCategory: itemCategories.get(item.category)
    }));
}

console.log(getCategorizedItems(items));

\$\endgroup\$
2
\$\begingroup\$

There is some very bad naming in the code. Names are too long and have overlapping abstractions. What is a category, there are two types a, b, or c and testnum1 and itemNum2

Due to poor naming the function has lost so much semantic accuracy it takes a second look to see what it is doing.

What are you doing?

getCategorizedItems getting categorized items??

  • From the input example all items are already categorized. So the function is not really getting categorized items

  • The output has new categories "a" becomes "test123" so the items are being recategorized (or because there is a one to one match the cats are being transformed)

  • Each item is restructured, From {name: 'test', value: 1, category: 'a'} to {name: 'test', category: 'testnum1'}

  • Items are filtered by existence of transformed category names.

Rename

Naming the function transformCategoriesRestructureAndFilter is impractical so recategorize could be more apt within the context of the code

As you are using category as an identifier, you should make it clear categoryId or catId, or even just id. Thus

// ignoring irrelevant data
itemList = {testnum1: {catId: 'a'}, testnum2: {catId: 'b'}}; 
items = [{name: '1', catId: 'a'},{name: '2', catId: 'b'},{name: '3', catId: 'c'}];

Use appropriate data structures

The object itemList is a transformation and should be a Map for quick lookup. Either defined as such

const catIdTransform = new Map((["a", "testnum1"], ["b", "testnum2"]]);

Or constructed from itemList

const catIdTransform = new Map(Object.entries(itemList).map(([name, {catId}]) => [catId, name]));

Rewrite

Thus you can rewrite using the new naming.

First transform the catId and then filter items without new ids.

function recategorize(items, idMap) {
    return items.map(item => ({...item, catId: idMap.get(item.catId)}))
        .filter(({catId}) => catId);
}

Working example

Using renamed data structures.

const items = [{name: '1', catId: 'a'},{name: '2', catId: 'b'},{name: '3', catId: 'c'}];
const catIdTransform = new Map([["a", "num1"], ["b", "num2"]]);

function recategorize(items, idMap) {
    return items.map(item => ({...item, catId: idMap.get(item.catId)}))
        .filter(({catId}) => catId);
}

console.log(recategorize(items, catIdTransform));

or if you are forced to maintain the naming and existing data

const items = [{name: '1', category: 'a'}, {name: '2', category: 'b'}, {name: '3', category: 'c'}];
const itemList = {testnum1: {category: 'a'}, testnum2: {category: 'b'}}; 
const categoryTransform = new Map(Object.entries(itemList).map(([name, {category}]) => [category, name]));

function categorizedItems(items, idMap) {  // at least drop the get
    return items.map(item => ({...item, category: idMap.get(item.category)}))
        .filter(({category}) => category);
}

console.log(categorizedItems(items, categoryTransform));

\$\endgroup\$

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.