JavaScript function to convert an array to an array of strings (formatted)

Question

Input

categories - an array of categories

[
    { id: 1, title: 'Smartphones' },
    { id: 2, title: 'Laptops' }
]

products - an array of products, linked to categories

[
    { id: 1, title: 'iPhone X', category_id: 1, price_min: 1200, price_max: 1700 },
    { id: 2, title: 'MacBook Pro Retina 13', category_id: 2, price_min: 1500, price_max: 4500 },
    { id: 3, title: 'Samsung Galaxy S8', category_id: 1, price_min: 0, price_max: 0 }
]

Output

[
    "*Smartphones*\niPhone X\n ($1200 - $1700)\nSamsung Galaxy S8 (free)",
    "*Laptops*\nMacBook Pro Retina 13 ($1500 - $4500)"
]

Current implementation

function printPriceList (categories, products) {
    return categories.map(category => {
        const pcontent = products
            .filter(product => product.category_id === category.id)
            .map(product => {
                const samePrice = product.price_min === product.price_max
                const price = samePrice ? product.price_min : `${product.price_min} - ${product.price_max}`

                if (price === 0) {
                    return `${product.title} (free)`
                }

                return `${product.title} ($${price})`
            })
        return [`*${category.title}*`].concat(pcontent)
    }).map(cat => cat.join('\n'))
}

I'd like to find out a way to refactor it to be more readable, easy to understand and efficient.

What can we use?

• all the modern JS features, like a spread operator (...smth)
• lodash

Answer 1

First impressions:

• too much nesting, too many nested returns
• inconsistent style, inconsistent naming

You already recognized the potential for refactoring. The simplest possible technique for refactoring is renaming variables:

• Your code currently uses camelCase (printPriceList, samePrice), underscores (category_id, price_min) and something else (pcontent). For me, it doesn't matter that much which style you chose as long as you use it consistently and as long as you use descriptive names.

Other stylistic suggestions:

• It is a habit of mine to split complex chains such as...

  const result = elements
      .filter(...)
      .map(...);
  

  ...into separate assignments with self-documenting names:

  const blabla = elements.filter(...);
  const result = blabla.map(...);
  

• I prefer to terminate statements with semicolons instead of having to think about the rules of JavaScript's automatic semicolon insertion.

• Instead of the 'print' prefix which signals some kind of active output e.g. to a stream, you could use e.g. 'format'.

The most helpful refactoring is to extract code fragments that can be grouped together into their own functions:

• Printing a product price:

  function print_product_price({price_min, price_max}) {
      if (price_max === 0) {
          return 'free';
      } else if (price_min === price_max) {
          return `$${price_min}`;
      } else {
          return `$${price_min} - $${price_max}`
      }
  }
  

• Printing a product:

  function print_product(product) {
      const price = print_product_price(product);
      return `${product.title} (${price})`; 
  }
  

• Printing a category:

  function print_category(category, products) {
      const category_products = products.filter(product => product.category_id === category.id);
  
      const category_rows = [
          `*${category.title}*`,
          ...category_products.map(print_product)
      ];
  
      return category_rows.join('\n');
  }
  

  Using Lodash, the above .filter expression could be written as _.filter(products, { 'category_id': category.id }) - suggested by @Gerrit0.

• And finally printing the price list:

  function print_price_list(categories, products) {
      return categories.map(category => print_category(category, products));
  }
  

As you can see, due to extracting those functions, no secondary or deeper nesting is needed.