REST web service in Express I have made a REST web service using the Express framework. Here is my snippet of code, which has repeated code. I am creating a currencyItem object in two blocks and push it into an array.

I think it is not bad practice. Can anyone suggest if it is OK? If not, how can I improve it?

/** API to get all currency price */
router.get("/:ticker?", function(req, res, next) {
  var currencyList = [];

  // it can be null as ticker is optional parameter
  let reqCurrencyName = req.params.ticker;

  rp(baseUrl)
    .then(body =&gt; {
      const $ = cheerio.load(body);
      $("tr").each(function name(i, elem) {
        // currency name
        var currencyName = $(this)
          .find("td")
          .eq(1)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim();

        // market cap value
        var marketCap = $(this)
          .find("td")
          .eq(2)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim();

        // currency price
        var price = $(this)
          .find("td")
          .eq(3)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim();

        // date supply value
        var dataSupply = $(this)
          .find("td")
          .eq(4)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim()
          .replace(/ /g, "");

        // volume value
        var volume = $(this)
          .find("td")
          .eq(5)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim();

        // negative change value
        var negativeChange = $(this)
          .find("td")
          .eq(6)
          .text()
          .replace(/\r?\n|\r/g, " ")
          .trim();

        // first item is empty in response so lets not push it into an array
        if (currencyName !== "") {
          if (reqCurrencyName !== undefined) {
            if (currencyName === reqCurrencyName) { // execute single time
              const currencyItem = {
                currencyName: currencyName,
                marketCap: marketCap,
                price: price,
                dataSupply: dataSupply,
                volume: volume,
                negativeChange: negativeChange
              };

              currencyList.push(currencyItem);
            }
          } else {
            const currencyItem = {
              currencyName: currencyName,
              marketCap: marketCap,
              price: price,
              dataSupply: dataSupply,
              volume: volume,
              negativeChange: negativeChange
            };

            currencyList.push(currencyItem);
          }
        }
      });
      res.send(currencyList);
    })
    .catch(err =&gt; {
      rollbar.log(err);
      res.send({ error: err });
    });
});
What you're essentially doing is transforming a list to another list (a list of &lt;tr&gt; to an array). Instead of jquery.each() and pushing to an array, use jquery.map() instead. jquery.map() works like array.map() except it returns a jQuery collection (an array-like object with jQuery metadata). You can bolt on jquery.get() at the end to convert it to a plain array afterwards.

There's also a shorter version of jquery.find - using the $ function with two arguments. The first being the selector, the second being the context. Internally, this just does jquery.find but from an authoring point of view, it's just shorter. In addition, you can use the :eq() selector in place of jquery.eq().

Now the above advice is for jQuery. I'm not exactly sure how identical the Cheerio API is to the jQuery API, but if they are, the above advice will probably hold.

As mentioned in the other answer, your code is heavily duplicated. A common advice is to move it out to a function and call that function. But you do not simply do that. I recommend taking a functional approach. You pass in all the arguments needed for the function to work, and operate on those arguments. This keeps the function independent from scope, variable visibility, side effects and so on.

Also suggesting using let and const whenever possible, preferrably const if you don't expect a variable's value to change. This signals other readers of your code what a variable should be, if it's supposed to be block-scoped or not, if the value can be changed or not.

There's also object property shorthands. If the variable you're assigning to an object property has the same name as the property, you can omit the : and the right-hand side.

Update: I've always been told (and I always forget) to pull filter operations early on in the stream. This reduces the number of iterations down the stream, avoiding unnecessary iterations. In this case, we filter out all entries without currency name first to avoid scraping and data generation for them.

const getText = element =&gt; element.text().replace(/\r?\n|\r/g, " ").trim();

router.get("/:ticker?", function(req, res, next) {    
  const reqCurrencyName = req.params.ticker

  rp(baseUrl).then(body =&gt; {
    const $ = cheerio.load(body);
    const currencies = $("tr").filter((i, e) =&gt; {
      // Filter rows with no currency name
      return Boolean(getText($('td:eq(1)', e)))
    }).map((i, e) =&gt; ({
      // Construct the object
      currencyName: getText($('td:eq(1)', e))
      marketCap: getText($('td:eq(2)', e))
      price: getText($('td:eq(3)', e))
      dataSupply: getText($('td:eq(4)', e)).replace(/ /g, "")
      volume: getText($('td:eq(5)', e))
      negativeChange: getText($('td:eq(6)', e))
    }))

    // Respond
    res.send(currencies.get())
  })
  .catch(error =&gt; {
    rollbar.log(error)
    res.send({ error })
  })
})