0
\$\begingroup\$

I'm trying to create a scraping API in Express. The API scrapes the different articles featured on the home page.

Here are the issues I'm trying to resolve:

  • My code's turning into a ton of jquery select statements. How can I resolve this / How can I simplify my code or extract functions from this giant mass?
  • Are my variable names clear enough?
  • Are there any Node conventions I'm not following? I've seen a lot of let and I'm using var. If I'm doing anything wrong let me know.

Here is the block of code I'm running:

var express = require('express');
var fs      = require('fs');
var request = require('request');
var cheerio = require('cheerio');
var app     = express();

app.get('/scrape', function(req, res){
  url = 'http://anawesomewebsite.com';

  request(url, function(error, response, html){
    if(!error){
      var $ = cheerio.load(html);

      var title, url, image;
      var articles = [];
      var article = { title : "", url : "", image_src : ""};

      //Get articles featured at top of page
      var i = 0;
      $(".articles li .img img").each(function(){
          var image_src = $(this).attr('src');
          article.image_src = image_src;
          articles[i] = article;
          i = i + 1;
      });

      i = 0;
      $(".articles li a[class='title']").each(function(){
         var title = $(this).text();
         var url = $(this).attr('href');
         article.title = title;
         article.url = "http://vegnews.com" + url;
         articles[i] = article;
         i = i + 1;
      });

      //Get articles featured at bottom of page
      var number_of_articles_beforehand = articles.length;
      i = number_of_articles_beforehand;

      $(".article .image img").each(function(){
        var image_src = $(this).attr('src');
        article.image_src = image_src;
        articles[i] = article;
        i = i + 1;
      });

      i = number_of_articles_beforehand;
      $(".article .image a").each(function(){
         var url = $(this).attr('href');
         article.url = url
         articles[i] = article;
         i = i + 1;
      })

      i = number_of_articles_beforehand;
      $(".article .title").each(function(){
        var title = $(this).text();
        article.title = title;
        articles[i] = article;
        i = i + 1;
      })

    }
    res.send(articles);
  })
})

app.listen('8081')
console.log('Magic happens on port 8081');
exports = module.exports = app;
\$\endgroup\$
  • \$\begingroup\$ fwiw, you can simplify your loops as $(".articles li .img img").each( function(i) { and not worry about incrementing the loop counter yourself. \$\endgroup\$ – Marc Rohloff Apr 3 '17 at 17:43
1
\$\begingroup\$

A good rule of thumb is to always start breaking code into smaller methods however in your case I suggest you change your logic. Instead of looping through each element updating a list of articles rather loop through each li creating a complete article something like:

request(url, function(error, response, html){
  if(!error){
    const $ = cheerio.load(html);
    let articles = $('.article li').map( (i, el) => getArticle($(el) );
    res.send(articles);
  }
});

function getArticle($el) {
  return {
     image_src: $el.find('.img img").attr('src'),
     title:     $el.find("a[class='title']").text(),
     url:       "http://vegnews.com" + $el.find("a[class='title']").attr(href),
 }
} 

(This will perform slightly worse but will be much easier to maintain imo)

btw there is a bug here:

  var article = { title : "", url : "", image_src : ""};
  ...
  $(".articles li .img img").each(function(){
      ...
      articles[i] = article;
  });

You are assigning (sharing) the same article over and over not creating a new one. Each time through the loop you just overwrite the attributes.

\$\endgroup\$
3
\$\begingroup\$
  1. As for the jQuery/function usage, there is definitely room for improvement but it would be helpful to see an example HTML element that contains the article img, title, and url to understand the page structure a bit.
  2. I think your variable names are clear, but could be simplified. Also, it is common practice to use Camel Case with the first letter lowercase for Node/Javascript instead of underscores between words.
    • For example, replace number_of_articles_beforehand with numArticlesBefore
  3. Here is an article I found helpful regarding var, let, const
    • https://medium.com/javascript-scene/javascript-es6-var-let-or-const-ba58b8dcde75
    • Javascript allows variable hoisting (click here for more info) with the keyword var whereas variables/identifiers declared as let or const do not hoist.
    • Personally, I have stopped using var altogether to help improve code readability as mentioned in the article above. Also, it is common practice to use const for the Node package require statements instead of var.
    • Remove the var title, url, image; line because you are only using those variables within their respective functions.
    • Lastly, I would encourage you to replace i = i + 1; with i++;. It does the same thing and is easier to read. (FYI, i--; would decrement i by 1).
\$\endgroup\$

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.