4
\$\begingroup\$

Is there any room for improvement on this code?

I use mechanize to get the links of a job listing web site. There are pages with pagination (when jobs > 25) and pages without.

If there is, then the link number 5 is labelled after "Next >" and link number 6 is "Last >>". In order to identify them, I make assertions on their name and act accordingly. I then check how many pages the pagination occurs, and then, taking the numbers, I scrape the pages one by one.

  def scrape_duty_station
    agent = Mechanize.new  
    page=agent.get(self.web_link)

    if page.links[5].text =~ /^next/i
      pagestart=page.links[5].href.match(/\d+/).to_s.to_i
      pageend=page.links[6].href.match(/\d+/).to_s.to_i
      @page=page.links[7...31]
      scrape_single_page
      (pagestart..pageend).each do |i|
        @page=agent.get(self.web_link+"/#{i}")
        @page=@page.links[7...31]
        scrape_single_page
      end  
    else
      @page=page.links[5...30]
      scrape_single_page
    end
  end

  def scrape_single_page
    @page.each do |i|
      p=Position.new
      p.title = i.text
      p.web_link = i.href
      p.job_id = i.href.match(/\d+/).to_s
      p.station = Station.where(:web_link => self.web_link).first
      p.save
    end
  end
\$\endgroup\$

migrated from stackoverflow.com Aug 11 '12 at 19:01

This question came from our site for professional and enthusiast programmers.

3
\$\begingroup\$

Here are some comments. - If you are only @page as a way to communicate with scarpe_single_page, why not make it a parameter?

def scrape_duty_station
    agent = Mechanize.new  
    page = agent.get(web_link)

Avoid extra indent when one of the cases is simpler and can be dealt with easily.

Also, about your ranges. They should really be named constants.

    return scrape_single_page(page.links[5...30]) unless page.links[5].text =~ /^next/i
    arr = [page] + (l_i(page,5)..l_i(page,6)).collect{|i|agent.get(web_link+"/#{i}")}
    arr.each {|p| scrape_single_page p.links[7...31] }
  end

If you are using any statement more than once, it may be worthwhile to facter it out to a function.

  def l_i(page, link)
    page.links[link].href.match(/\d+/).to_s.to_i
  end

Does position have a constructor? If not, it would be nice to create one for it. If yes, it would be better to use it.

  def scrape_single_page(pages)
    pages.each do |p|
      Position.new(p.text,p.href,p.href.match(/\d+/).to_s,
            Station.where(:web_link => self.web_link).first).save
    end
  end
\$\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.