REST web service in Express

Question

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 });
 });
});

• Mast: Is there a reason you put it all one big function?
• N Sharma: @Mast I am creating rest services using express first time. I don't know it is good or bad. can you please suggest

Answer 1

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 })
 })
})

• Igor Soloydenko: You missed .replace(/ /g, "") on field "4". Nice object construction syntax -- totally forgot about it
• N Sharma: I am getting SyntaxError: unmatched pseudo-class :eq
• Joseph: @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

Answer 2

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 });
 });
});

• N Sharma: Hi Igor, Can you please suggest why getFieldalways return empty value.
• Igor Soloydenko: @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.