Financial website scraper

Question

I wrote the code that would scrape Finviz for tickers and href that attach to the ticker. I tested with multi-page as well as one page. I'm new to Python programming and therefore believe my code is terrible. I'm hoping someone would help me review it and point out ways to improve it. My goal is to have the code more scalable than what I have now.

import bs4 as bs
import requests

class finviz_crawler():
    def requesting(self,type):
        items=1
        baseurl='https://finviz.com/screener.ashx?v=111'
        if type == 'sp500':
            firstpage=baseurl+'&f=idx_sp500&o=ticker'
        elif type == "China":
            firstpage=baseurl+'&f=cap_largeover,geo_china'
        finalurl=firstpage
        headers = {'User-Agent':'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36'}
        resp=requests.get(finalurl,headers=headers)
        soup = bs.BeautifulSoup(resp.text,"lxml")
        table=soup.findAll('td',{'class':'screener-body-table-nw'})
        maxpage_a_tag=[]
        maxpage_td=soup.findAll('td',{'class':'body-table'},bgcolor="#ffffff",valign="top",width="100%")
        tickerdict={}
        for tags in maxpage_td[0].findAll('a'):
             maxpage_a_tag.append(tags)
        if not maxpage_a_tag:
            maxpage=1
        elif (maxpage_a_tag[-1].text=="next"):
            maxpage=maxpage_a_tag[-2].text
        else:
            maxpage=maxpage_a_tag[-1].text
        for page in range(int(maxpage)):
            resp=requests.get(finalurl,headers=headers)
            soup = bs.BeautifulSoup(resp.text,"lxml")
            table=soup.findAll('td',{'class':'screener-body-table-nw'})
            for row in table:
                 ticker_rows=row.findAll('a',{'class':'screener-link-primary'},href=True)
                 for tickers in ticker_rows:
                     tickerdict[tickers.text]=tickers['href']
            items+=20
            finalurl="{}{}{}".format(firstpage,"&r=",str(items))
        print(tickerdict)

Call the class using finviz_crawler().requesting('China')

Answer 1

Here a few suggestions (in no particular order) for improvements on your code:

Make variables have locality

The line:

items = 1

is at the top of the method, but is only used by the loop at the bottom of the method. It is likely better located just in front of the loop that increments it.

Same applies to:

tickerdict = {}

You can break strings across multiple lines:

This:

headers = {
    'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36'}

Can be:

headers = {
    'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) '
    'AppleWebKit/537.36 (KHTML, like Gecko) '
    'Chrome/56.0.2924.87 Safari/537.36'
}

You often don't need loops:

for tags in maxpage_td[0].findAll('a'):
    maxpage_a_tag.append(tags)

likely works just fine as:

maxpage_a_tag.extend(maxpage_td[0].findAll('a'))

And then this:

maxpage_a_tag = []
maxpage_a_tag.extend(maxpage_td[0].findAll('a'))

is really just:

maxpage_a_tag = list(maxpage_td[0].findAll('a'))

Don't use python names

The name type is used by Python. Suggest instead changing to type_ or better yet, as suggested from comments, something more descriptive, like request_type.

Reworked code:

Here is the reworked code with the above suggestions, and maybe a few others that snuck in.

class finviz_crawler():

    def requesting(self, type_):
        baseurl = 'https://finviz.com/screener.ashx?v=111'
        if type_ == 'sp500':
            firstpage = baseurl + '&f=idx_sp500&o=ticker'
        elif type_ == "China":
            firstpage = baseurl + '&f=cap_largeover,geo_china'
        else:
            raise ValueError("Unknown type {}".format(type_))
        finalurl = firstpage
        headers = {
            'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) '
                          'AppleWebKit/537.36 (KHTML, like Gecko) '
                          'Chrome/56.0.2924.87 Safari/537.36'}
        resp = requests.get(finalurl, headers=headers)
        soup = bs.BeautifulSoup(resp.text, "lxml")
        maxpage_td = soup.findAll('td', {'class': 'body-table'},
                                  bgcolor="#ffffff", valign="top",
                                  width="100%")
        maxpage_a_tag = list(maxpage_td[0].findAll('a'))
        if not maxpage_a_tag:
            maxpage = 1
        elif (maxpage_a_tag[-1].text == "next"):
            maxpage = maxpage_a_tag[-2].text
        else:
            maxpage = maxpage_a_tag[-1].text

        items = 1
        tickerdict = {}
        for page in range(int(maxpage)):
            resp = requests.get(finalurl, headers=headers)
            soup = bs.BeautifulSoup(resp.text, "lxml")
            table = soup.findAll('td', {'class': 'screener-body-table-nw'})
            find_all_args = 'a', {'class': 'screener-link-primary'}
            for row in table:
                tickerdict.update(
                    {tickers.text: tickers['href']
                     for tickers in row.findAll(*find_all_args, href=True)})
                items += 20
            finalurl = "{}{}{}".format(firstpage, "&r=", str(items))

        for data in tickerdict.items():
            print(data)


finviz_crawler().requesting('China')

Answer 2

In addition to what @StephenRauch wrote in his excellent answer, let me add one more comment:

Your class is completely useless. Not its functionality, which is fine, but it being a class is completely unnecessary.

It does not inherit anything from a parent or has children which inherit from it, it has no attributes and therefore no state, and the one method it does have does not even use self anywhere. It uses none of the features of OOP.

In other words, you might as well make that method a stand-alone function. And while you're at it, maybe make it slightly more reusable by passing baseurl as a parameter.

A worthwhile related video: Stop Writing Classes

---

Also, don't build your URL by hand, use the params keyword of requests.get:

type_params = {'sp500': {'f': 'idx_sp500', 'o': 'ticker'},
               'China': {'f': 'cap_largeover,geo_china'}}

url = 'https://finviz.com/screener.ashx'
params = {'v': 111}
params.update(type_params[type])
headers = {'User-Agent': ...}

resp = requests.get(url, headers=headers, params=params)

This even takes care of correctly quoting the values, if necessary.

Answer 3

You should also split the code in functional parts, and not mix the IO (print(...)) with generating the result

base_url

First you generate the base_url

def make_base_url(request_type):
    baseurl='https://finviz.com/screener.ashx?v=111'
    if request_type == 'sp500':
        firstpage=baseurl+'&f=idx_sp500&o=ticker'
    elif request_type == "China":
        firstpage=baseurl+'&f=cap_largeover,geo_china'
    # What will happen when request_type is not `sp500` or `China`
    return firstpage  

Behaviour when input is not sp500 or China is undefined, this will trow an exception when that happens

get_soup

HEADERS_DEFAULT = {'User-Agent':'Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/56.0.2924.87 Safari/537.36'}    

def get_soup(url, headers=HEADERS_DEFAULT):
    resp=requests.get(url,headers=headers)
    return bs.BeautifulSoup(resp.text,"lxml")

This will, when presented an url, get the soup

find the max_page

def find_max_page(base_soup):
    td_s = base_soup.findAll('td',{'class':'body-table'},bgcolor="#ffffff",valign="top",width="100%")
    a_s = td_s[0].findAll('a')
    if not a_s:
#         print('found nothing')
        return 1
    max_page = a_s.pop().text
    return int(max_page) if max_page != 'next' else int(a_s.pop().text)

Do the int conversion where it's most logical

Find the URLs for the next pages

def find_page_urls(base_url, max_pages):
    yield base_url
    for i in range(max_pages):
        items = str(1 + (i + 1) * 20)
        yield "{}&r={}".format(base_url, items)

Extract the ticker

def get_ticker(url, headers=HEADERS_DEFAULT):
    soup = get_soup(url, headers)
    table=soup.findAll('td',{'class':'screener-body-table-nw'})
    for row in table:
         ticker_rows=row.findAll('a',{'class':'screener-link-primary'},href=True)
         for tickers in ticker_rows:
             yield tickers.text, tickers['href']

get the results

def find_pages(urls, headers=HEADERS_DEFAULT):
    results = dict()
    for url in urls:
        results.update(get_ticker(url, headers))
    return results

putting it together

base_url = make_base_url('China')
base_soup = get_soup(base_url)
max_pages = find_max_page(base_soup)

urls = find_page_urls(base_url, max_pages, headers=HEADERS_DEFAULT)
result_dict = find_pages(urls)

If you structure it like this, you can test each of the portions separately, and debug it manually if needed