4
\$\begingroup\$

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 => {
      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 => {
      rollbar.log(err);
      res.send({ error: err });
    });
});
\$\endgroup\$
  • 2
    \$\begingroup\$ Is there a reason you put it all one big function? \$\endgroup\$ – Mast Sep 11 '17 at 18:17
  • \$\begingroup\$ @Mast I am creating rest services using express first time. I don't know it is good or bad. can you please suggest \$\endgroup\$ – N Sharma Sep 11 '17 at 18:21
3
\$\begingroup\$

What you're essentially doing is transforming a list to another list (a list of <tr> 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 => element.text().replace(/\r?\n|\r/g, " ").trim();

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

  rp(baseUrl).then(body => {
    const $ = cheerio.load(body);
    const currencies = $("tr").filter((i, e) => {
      // Filter rows with no currency name
      return Boolean(getText($('td:eq(1)', e)))
    }).map((i, e) => ({
      // 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 => {
    rollbar.log(error)
    res.send({ error })
  })
})
\$\endgroup\$
  • 1
    \$\begingroup\$ You missed .replace(/ /g, "") on field "4". Nice object construction syntax -- totally forgot about it \$\endgroup\$ – Igor Soloydenko Sep 11 '17 at 21:33
  • \$\begingroup\$ I am getting SyntaxError: unmatched pseudo-class :eq \$\endgroup\$ – N Sharma Sep 12 '17 at 19:40
  • \$\begingroup\$ @Williams As mentioned, the above advice is for jQuery and may not apply to Cheerio. Adjust as needed. It looks like Cheerio does not implement :eq(). However, it does appear to support :nth-child() which is close (but not identical) to :eq(). See github.com/cheeriojs/cheerio#-selector-context-root- and github.com/fb55/css-select#supported-selectors \$\endgroup\$ – Joseph Sep 12 '17 at 20:47
2
\$\begingroup\$

A few suggestions to translate 95 lines into 34:

  • DRY -- why do $(this).find(...)... so many times?! Extract into a function.
  • Do NOT use var/let. DO use const instead.
  • Both branches of the if (reqCurrencyName !== undefined) { ... } else { ... } are doing same exact thing -- no need to branch the logic therefore.
  • DO spell things out. Names like res and err are as bad as rp(). Is res a "response" or "result" or "rescue" or "resource" or "resume" or ... -- you get it.
  • I am not familiar with cheerio, but I suspect that if you're iterating over TRs: $("tr").each((_, elem) => { ... }), you could replace $(this).find("td") with something like elem.find("td") just figure out what's been given to you in the each method.

router.get("/:ticker?", function(request, response, next) {
    const currencyList = [];
    const requestCurrencyName = request.params.ticker;
    rp(baseUrl)
        .then(body => {
            const $ = cheerio.load(body);
            const getField = orderNumber => $(this).find("td").eq(orderNumber).text().replace(/\r?\n|\r/g, " ").trim();

            $("tr").each((_, elem) => {
                const currencyName = getField(1);
                const marketCap = getField(2);
                const price = getField(3);
                const dataSupply = getField(4).replace(/ /g, "");
                const volume = getField(5);
                const negativeChange = getField(6);

                if (currencyName !== '') {
                    const currencyItem = {
                        currencyName: currencyName,
                        marketCap: marketCap,
                        price: price,
                        dataSupply: dataSupply,
                        volume: volume,
                        negativeChange: negativeChange
                    };
                    currencyList.push(currencyItem);
                }
            });
            response.send(currencyList);
        })
        .catch(error => {
            rollbar.log(error);
            response.send({ error });
        });
});
\$\endgroup\$
  • \$\begingroup\$ Hi Igor, Can you please suggest why getFieldalways return empty value. \$\endgroup\$ – N Sharma Sep 13 '17 at 4:48
  • \$\begingroup\$ @Williams it won't be easy to debug that unless provided with a plnkr. I think, I might messed up the getField in the $(this) part. You could try to do something like const getField = (elem, orderNumber) => $(elem).find("td").eq(orderNumber).text().replace(/\r?\n|\r/g, " ").trim(); instead, but the consumers would need to be changed too (getField(elem, 3)). My answer was not intended to provide a solution that guaranteed to work, but to give the idea(s) how to improve your code. If I had a plunkr to start with, I could fix my answer. \$\endgroup\$ – Igor Soloydenko Sep 13 '17 at 5:10

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.