2
\$\begingroup\$

I am working on getting more experience with Python so I made a basic Web Scraper, that saves it to MySQL.

The code is currently working, but looking at it is very frustrating because it seems extremely redundant to me.

Some fake html - This does emulate the sites tree.

 <div class="tab_row">
  <h4>World</h4>
  <div class="discount_original_price">$1.99</div>
  <div class="discount_final_price">$1.99</div>
 </div>
 <div class="tab_row">
  <h4>World2</h4>
  <div class="discount_original_price">$2.99</div>
  <div class="discount_final_price">$2.99</div>
 </div>
 <div class="tab_row">
  <h4>World3</h4>
  <div class="discount_original_price">$3.99</div>
  <div class="discount_final_price">$3.99</div>
 </div>
 <div class="tab_row">
  <h4>World4</h4>
  <div class="discount_original_price">$4.99</div>
  <div class="discount_final_price">$3.99</div>
 </div>

My Python Script

from bs4 import BeautifulSoup
import urllib2
import csv
import dbconfig as cfg
origPage = 'http://localhost'
page = urllib2.urlopen(origPage)
import sys
import MySQLdb as mdb
reload(sys)
sys.setdefaultencoding('UTF8')

def pullGames():
 soup = BeautifulSoup (page, 'html.parser')
 #(page, 'html.parser')

 #Let's pull prices
 names = []
 pricing = []
 discounts = []
 discountPct = []
 #Push items to array for saving
 for name in soup.find_all('div', attrs={'class': 'tab_row'}):
  names.append(name.h4.text)
 for price in soup.find_all('div', attrs={'class': 'discount_original_price'}):
  pricing.append(price.text)
 for discount in soup.find_all('div', attrs={'class': 'discount_final_price'}):
  discounts.append(discount.text)
 for percent in soup.find_all('div', attrs={'class': 'discount_pct'}):
  discountPct.append(percent.text)


 con = mdb.connect(cfg.mysql['host'], cfg.mysql['user'], cfg.mysql['passwd'], cfg.mysql['db']);
 cur = con.cursor()
 con.set_character_set('utf8')
 cur.execute('SET NAMES utf8;')
 cur.execute('SET CHARACTER SET utf8;')
 cur.execute('SET character_set_connection=utf8;')


 for i in range(len(names)):
  sql = 'INSERT INTO game_pricing(game_name, original_price, discount_price) \
         VALUES("%s", "%s", "%s")' % \
         (names[i],pricing[i],discounts[i])
 #print sql
  cur.execute(sql)      
  con.commit()

def checkDb():
 con = mdb.connect(cfg.mysql['host'], cfg.mysql['user'], cfg.mysql['passwd'], cfg.mysql['db']);
 cur = con.cursor()
 con.set_character_set('utf8')
 cur.execute('SET NAMES utf8;')
 cur.execute('SET CHARACTER SET utf8;')
 cur.execute('SET character_set_connection=utf8;')
 sql = 'SELECT game_name, original_price, discount_price FROM game_pricing'
 cur.execute(sql)
 results = cur.fetchall()
 for row in results:
  game_name = row[0]
  original_price = row[1]
  discount_price = row[2]
  print "game_name=%s, original_price=%s, discount_price=%s" % \
         (game_name, original_price, discount_price)
#For testing
checkDb()

It currently succeeds and gets 2,800 records in just a few(~15) seconds, including DB save. I am now adding in additional code to review if an entry exists, and only update if they exist.

I wrapped both parts into functions so I can easily test them out without writing duplicates.

Welcome any feedback. While this is working, I am not even sure how to begin refactoring it for redundancy sake.

\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

Here are some notes grouped by categories.

HTML Parsing

Instead of collecting names and amounts in separate lists, iterate over the elements with a tab_row class and collect the results into a single list of dictionaries.

You can get performance improvements by switching to lxml from html.parser. Requires lxml to be installed.

You can use more concise CSS selectors instead of find_all().

You can also use SoupStrainer class that would allow to improve on parsing speed by limiting what the parsing tree to the desired part only.

Working with the database

First of all, do not use string formatting to pass your query parameters into the query. You are not only exposing your code to SQL injection attacks, but also get potential issues with character escaping and type conversions. Use parameterized queries instead.

If you are gonna collect your data into the list of dictionaries, you may do your insert via executemany() which, aside from being more concise, may provide a performance boost as well.

It is also a good idea to use multi-line strings for your database queries.

Code Style

There are multiple code style issues. The code is not PEP8-complaint. Please analyze your code with flake8, pylint or other linters.

Also, don't repeat the database connection code, extract it into a separate function and reuse.


Here is the modified version of the code (renamed functions and logically separated into multiple functions):

import sys
import urllib2

import MySQLdb as mdb
from bs4 import BeautifulSoup, SoupStrainer

import dbconfig as cfg


def db_connect():
    connection = mdb.connect(cfg.mysql['host'], cfg.mysql['user'], cfg.mysql['passwd'], cfg.mysql['db'])
    cursor = connection.cursor()
    
    connection.set_character_set('utf8')
    cursor.execute('SET NAMES utf8;')
    cursor.execute('SET CHARACTER SET utf8;')
    cursor.execute('SET character_set_connection=utf8;')

    return connection, cursor


def pull_games(page):
    parse_only = SoupStrainer(class_='tab_row')
    soup = BeautifulSoup(page, 'lxml', parse_only=parse_only)

    return [
        {
            'name': row.h4.get_text(),
            'price': row.select_one(".discount_original_price").get_text(),
            'discount': row.select_one('.discount_final_price').get_text()
        }
        for row in soup.select(".tab_row")
    ]


def insert_games(connection, cursor, data):
    query = """
        INSERT INTO 
          game_pricing
          (game_name, original_price, discount_price)
      VALUES
          (%(name)s, %(price)s, %(discount)s)
    """
    cursor.executemany(query, data)
    connection.commit()


def check_games(cursor):
    query = """
        SELECT 
            game_name, original_price, discount_price 
        FROM 
            game_pricing
    """
    cursor.execute(query)

    result = cursor.fetchall()
    for game_name, original_price, discount_price in result:
        print("game_name=%s, original_price=%s, discount_price=%s" % (game_name, original_price, discount_price))


if __name__ == '__main__':
    reload(sys)
    sys.setdefaultencoding('UTF8')

    url = 'http://localhost'
    page = urllib2.urlopen(url)

    games = pull_games(page)

    try:
        connection, cursor = db_connect()
        insert_games(connection, cursor, games)
    
        check_games(cursor)
    finally:
        cursor.close()
        connection.close()
\$\endgroup\$
2
  • \$\begingroup\$ Wow, thanks a lot for the feedback. I hated my formatting too, but I was making such good progress on the script I wanted to complete it prior to looking up best-practices. I will review the compliant document. I have a question about the way your document ends - I've seen this on a lot of Py scripts. What is the if __name__ == etc function doing it? It seems like it's just a way to specify 'This is what should be ran on open'? \$\endgroup\$
    – DNorthrup
    Jul 15, 2017 at 15:59
  • \$\begingroup\$ @DNorthrup right, if __name__ == '__main__': would help to avoid the code to be executed on import, but also has an code organizational impact - note how we've put all the top-level code there into a single place. Thanks. Hope that was helpful. \$\endgroup\$
    – alecxe
    Jul 15, 2017 at 16:11

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.