2
\$\begingroup\$

I recently submitted a code sample for a web scraping project and was rejected without feedback as to what they didn't like. The prompt, while I cannot give it here verbatim, basically stated that I needed to write a spider to crawl a site for product items. They suggested using a generic spider to scrape the site in question while using URL rules for efficiency. They gave links to documentation in case you hadn't used scrapy before. I felt like this meant that they didn't mind hiring people unfamiliar with their toolset.

Speaking of which we could only use pyquery for dom traversal. I usually would have opted for pure lxml and xpaths.

I understood the concept of using rules to limit extraneous requests but after noticing that the site in question contained a sitemap I decided to start there instead.

I do know that they explicitly said not to use any outside libraries, so that is why I didn't use Pillow for image processing. However, I did cheat and use requests for some other things that the actual spider didn't utilize but again I wasn't told why my code wasn't good enough. So at this point I would like to learn why.

# -*- coding: utf-8 -*-
import scrapy
from scrapy.spiders.sitemap import *
from pyquery import PyQuery as pq
from oxygendemo.items import OxygendemoItem
import oxygendemo.utilities
from oxygendemo.utilities import *


class OxygenSpider(SitemapSpider):

    print 'MY SPIDER, IS ALIVE'
    name = "oxygen"
    allowed_domains = ["oxygenboutique.com"]
    sitemap_urls = ['http://www.oxygenboutique.com/sitemap.xml']
    sitemap_rules = generate_sitemap_rules()
    ex_rates = get_exchange_rates()

    def parse_sitemap_url(self, response):

        self.logger.info('Entered into parse_sitemap_url method')
        self.logger.info('Received response from: {}'.format(response.url))
        self.logger.debug('Respons status: {}'.format(response.status))

        item = OxygendemoItem()
        d = pq(response.body)
        parsed_url = urlparse.urlparse(response.url)
        base_url = get_base(parsed_url)
        product_info = d('.right div#accordion').children()
        image_links = d('div#product-images tr td a img')
        description = product_info.eq(1).text()\
            .encode('ascii', 'ignore')
        item['code'] = str(parsed_url[2].lstrip('/')[:-5])
        item['description'] = description
        item['link'] = parsed_url.geturl()
        item['name'] = d('.right h2').text()

        gbp_price = {
            'prices': d('.price').children(),
            'discount': 0
            }

        item['gbp_price'], item['sale_discount'] = get_price_and_discount(
            gbp_price
        )

        if 'error' not in self.ex_rates:

            item['usd_price'] = "{0:.2f}".format(
                item['gbp_price'] * self.ex_rates['USD']
                )
            item['eur_price'] = "{0:.2f}".format(
                item['gbp_price'] * self.ex_rates['EUR']
                )
        else:

            item['usd_price'], item['eur_price'] = ['N/A'] * 2

        item['designer'] = d('.right').find('.brand_name a').text()
        item['stock_status'] = json.dumps(determine_stock_status(d('select')
                                          .children()))
        item['gender'] = 'F'  # Oxygen boutique carries Womens's clothing
        item['image_urls'] = fetch_images(image_links, base_url)
        item['raw_color'] = get_product_color_from_description(description)

        yield item

This is the utilities module I used:

# -*- coding: utf-8 -*-
import requests
import json
import urlparse
from pyquery import PyQuery as pq
import re


def get_base(parsed_url):

    base_url = parsed_url[0] + '://' + parsed_url[1]
    base_url = base_url.encode('ascii', 'ignore')
    return base_url


def get_exchange_rates():
    ''' return dictionary of exchange rates with british pound as base
        currency '''

    url = 'http://api.fixer.io/latest?base=GBP'
    try:
        response = requests.get(url)
        er = json.loads(response.content)['rates']
        return er
    except:
        return {'error': 'Could not contact server'}


def determine_stock_status(sizes):

    result = {}
    for i in xrange(1, len(sizes)):
        option = sizes.eq(i).text()
        if 'Sold Out' not in option:
            result[option] = 'In Stock'
        else:
            size = option.split(' ')[0]
            result[size] = 'Sold Out'
    return result


def determine_type(short_summary):

    short_summary = short_summary.upper()

    S = {
        'HEEL', 'SNEAKER', 'SNEAKERS',
        'BOOT', 'FLATS', 'WEDGES',
        'SANDALS'
    }

    J = {
        'RING', 'NECKLACE', 'RING',
        'BANGLE', 'CHOKER', 'COLLIER',
        'BRACELET', 'TATTOO', 'EAR JACKET'
    }

    B = {
        'BAG', 'PURSE', 'CLUTCH',
        'TOTE'
    }

    A = {
        'PINNI', 'BLOUSE', 'TOP',
        'SKIRT', 'KNICKER', 'DRESS',
        'DENIM', 'COAT', 'JACKET',
        'SWEATER', 'JUMPER', 'SHIRT',
        'SKINNY', 'SHORT', 'TEE',
        'PANTS', 'JUMPSUIT', 'HIGH NECK',
        'GOWN', 'TROUSER', 'ROBE',
        'PLAYSUIT', 'CULOTTE', 'JODPHUR',
        'PANTALON', 'FLARE', 'CARDIGAN',
        'VEST', 'CAMI', 'BEDSHORT',
        'PYJAMA', 'BRALET', 'TUNIC',
        'HOODY', 'SATEEN', 'BIKER',
        'JEAN', 'SWEAT', 'PULL',
        'BIKINI', 'LE GRAND GARCON'
    }

    types = {
        'B': B, 'S': S,
        'J': J, 'A': A
    }

    for key, val in types.iteritems():
        for t in val:
            if t in short_summary:
                return key
    else:
        return 'R'  # Tag as accessory as failsafe


def fetch_images(image_links, base_url):
    ''' base_url will come as unicode change to python string '''

    images = []

    for image in image_links:
        images.append(urlparse.urljoin(base_url, image.attrib['src']))

    return images


def get_price_and_discount(gbp_price):

    if gbp_price['prices']('.mark').text() == '':  # No discount
        gbp_price['discount'] = '0%'
        orig_price = float(gbp_price['prices'].parent().text()
                           .encode('ascii', 'ignore'))
    else:  # Calculate discount
        prices = gbp_price['prices']
        orig_price = "{0:.2f}".format(float(prices('.mark').text()))
        new_price = "{0:.2f}".format(float(gbp_price['prices'].eq(1).text()))
        gbp_price['discount'] = "{0:.2f}"\
            .format(float(orig_price) / float(new_price) * 100) + '%'
    return float(orig_price), gbp_price['discount']


def get_raw_image_color(image):
    ''' Note that Pillow imaging library would be perfect
        for this task. But external libraries are not
        allowed via the constraints noted in the instructions.
        Example: Image.get_color(image)
        Could be used with Pillow.
    '''

    # only import Pillow image library if this is used
    # Later
    from PIL import Image
    im = Image.open(image)
    colors = im.getcolors()
    if colors is None:
        return None
    else:
        return colors[0]  # Not functional at this point


def get_product_color_from_description(description):
    ''' Will go this route to avoid external imports '''

    description = description.upper().split(' ')
    colors = (
        'BLACK', 'WHITE', 'BLUE',
        'YELLOW', 'ORANGE', 'GREY',
        'PINK', 'FUSCIA', 'RED',
        'GREEN', 'PURPLE', 'INDIGO',
        'VIOLET'
    )

    for word in description:
        for color in colors:
            if word == color:
                return color.lower()
    else:
        return None


def generate_sitemap_rules():

    d = pq(requests.get('http://www.oxygenboutique.com').content)

    # Proof of concept regex can be found here --> http://regexr.com/3c0lc
    designers = d('ul.tame').children()
    re_front = r'(http:\/\/)(www\.)(.+\/)((?!'
    re_back = r').+)'
    re_middle = 'products|newin|product|lingerie|clothing'

    for li in designers:
        ''' This removes 36 requests from the queue '''

        link = pq(li.find('a')).attr('href').rstrip('.aspx')
        re_middle += '|' + link

    return [(re_front + re_middle.replace('-', r'\-') + re_back,
            'parse_sitemap_url')]

OxygendemItem() declaration:

import scrapy
from scrapy import Field


class OxygendemoItem(scrapy.Item):

    code = Field()  # unique identifier (retailers perspective)
    description = Field()  # Detailed description
    designer = Field()  # manufacturer
    eur_price = Field()  # full (non_discounted) price
    gender = Field()  # F - Female, M - male
    gbp_price = Field()  # full (non_discounted) price
    image_urls = Field()  # list of urls representing the item
    link = Field()  # url of product page
    name = Field()  # short summary of the item
    raw_color = Field()  # best guess of color. Default = None
    sale_discount = Field()  # % discount for sale item where applicable
    stock_status = Field()  # dictionary of sizes to stock status
    '''
                   size: quantity
        Example: { 'L': 'In Stock',
                   'M': 'In Stock',
                   'S': 'In Stock',
                   'XS': 'In Stock'
                 }
    '''
    # 'A' = apparel, 'B' = bags, 'S' = shoes, 'J' = jewelry, 'R' = accessories
    type = Field()
    usd_price = Field()  # full (non_discounted) price
\$\endgroup\$
3
\$\begingroup\$

Well to start with you have bad practices in your imports. It's recommended to stay away from using from module import * because doing that imports things without explicitly declaring their names. Without realising it, you could be overwriting other functions, including builtins in the module was made carelessly. Instead use just import module or from module import func1, func2, CONST. Especially though, don't do this:

import oxygendemo.utilities
from oxygendemo.utilities import *

It's totally redundant to have the first line since you're then ignoring it to import everything. In case you don't know, you can still alias plain imports:

import oxygendemo.utilities as util

So you don't even need to worry about the name being too long.

Also OxygenSpider is not laid out properly. You have loose code that should probably be in an __init__ function. Let me show you how this works in the interpreter:

>>> class A:
    print "Printing class A"


Printing class A

So what happened there? The print command was run when the class was created. I haven't created any object yet, so what happens when I create an object:

>>> A()
<__main__.A instance at 0x0000000002CA5588>
>>> b = A()
>>> 

Nothing. It's not printing the command that you intended to appear when creating an OxygenSpider object. If you were to wrap it in __init__ though, it would. __init__ is a special function that runs when a new object is created, like so:

>>> class A:
    def __init__(self):
        print "Printing this object"

>>> A()
Printing this object
<__main__.A instance at 0x0000000002113488>
>>> b = A()
Printing this object

You see now? Nothing happens after the class is created but when actual objects are created __init__ gets run. You should be putting the whole opening block to OxygenSpider in a function like that. Also the variables should be assigned as self.var, and the constants should be in UPPER_SNAKE_CASE and constant lists should be tuples instead. Tuples are made with () and are basically like lists except they cannot be changed.

However since you're inheriting from SitemapSpider you also need to run its __init__ function in yours. You need to call it so that your base class is initialised before you run your particular __init__ code. There's a good explanation in this Stack Overflow answer

class OxygenSpider(SitemapSpider):

    def __init__(self):
        super(SitemapSpider, self).__init__()
        print 'MY SPIDER, IS ALIVE'
        self.NAME = "oxygen"
        self.ALLOWED_DOMAINS = ("oxygenboutique.com")
        self.SITEMAP_URLS = ('http://www.oxygenboutique.com/sitemap.xml')
        self.sitemap_rules = generate_sitemap_rules()
        self.ex_rates = get_exchange_rates()

Also printing when creating an object just to say it's created isn't very nice anyway, you should remove that.

| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Ahh ok thanks for that @SuperBiasedMan. I tried doing declaring instance variables with self initially but then I ran into self not defined error. But I guess that wouldn't have happened had I used __init__() properly. \$\endgroup\$ – trendsetter37 Oct 23 '15 at 13:56
  • \$\begingroup\$ @trendsetter37 Yeah, for self to work it needs to be in the function, since __init__ takes self as the parameter you can refer to. \$\endgroup\$ – SuperBiasedMan Oct 23 '15 at 14:02
  • 2
    \$\begingroup\$ placing the name attribute within the init method yields a spider not found error. In the docs they declare name outside of init in their examples. Is that absolutely necessary or does their codebase require that for everything to work? \$\endgroup\$ – trendsetter37 Oct 23 '15 at 14:54
  • 2
    \$\begingroup\$ @trendsetter37 This is because you're now overriding their __init__ method which declare a spider and launch all the logic. Even though it is good practice to use an __init__ to initialize things, in this case it is just not what you should do. \$\endgroup\$ – 301_Moved_Permanently Oct 23 '15 at 15:39
  • 1
    \$\begingroup\$ Argh. Apologies, I forgot about the inheritance. I added a note about calling __init__. It should work fine with the added line that calls on SitemapSpider's __init__. \$\endgroup\$ – SuperBiasedMan Oct 23 '15 at 16:09
3
\$\begingroup\$

Your utilities module is too specific

Separating the code into data model, utilities functions and proper logic is generally a good idea. But in your code, the utilities module contains functions that are tightly coupled with your spider. I’d rather use them as @staticmethods of your spider.

Except maybe for get_exchange_rates and get_base.

However, for such an exercise, I’d put them all into a single file since it’s easier to handle and reduce import cluttering.

Stay consistent

Whatever coding style you choose to apply, stay consistent. For instance, in your items.py file, you define class OxygendemoItem(scrapy.Item): and then your define your fields using Field(). Either use scrapy.Item and scrapy.Field() or Item and Field() but do not mix them. Same for the indentation level of closing parenthesis or braces…

Use higher-level (“more pythonic”) constructs

Some looping constructs would benefit from using the right data structure to simplify the logic.

Compare your fetch_images with:

[urlparse.urljoin(base_url, image.attrib['src']) for image in image_links]

no need to define a function anymore. Same for your get_product_color_from_description:

description = set(description.upper().split())
colors = {
    'BLACK', 'WHITE', 'BLUE',
    'YELLOW', 'ORANGE', 'GREY',
    'PINK', 'FUSCIA', 'RED',
    'GREEN', 'PURPLE', 'INDIGO',
    'VIOLET'
}
try:
    return (description & colors).pop()
except KeyError:
    return None

By the way, for this one:

  • colors should be a global variable; same for other collections you build elsewhere in your utilities module;
  • use split without parameters, it will remove multiple blanks (spaces, tabs, newlines) before splitting the string;
  • remember to always put an exception type in the except clause (unlike what you did with get_exchange_rates).

Use what you are told to use

That might be the root cause of them not even giving feedback to you. For whatever reason they have their environment set-up so that a small amount of libraries are available, if you provide them code that requires external modules it won't run. Why would they take time to answer someone that didn't take time to test its code before submitting it?

The worse part is get_raw_image_color. You're not even calling it. If you truly want to explain that you know how to use an external tool that would better fit for the task, put it as a comment of your submission. But in the code it has no added value, it's just noise.

[Speculative] Use scrapy builtin capabilities

As far as I understand (I'm not a scrapy user), scrapy installation requires lxml and provides DOM manipulation through xpath and css selectors. If you are more used to them, go for it.

I don't know if scrapy can be installed without lxml, though, so I might stick with PyQuery just in case.

Examine the problem

Even though you were told to use url rules, they are not really of added value here. Looking at the sitemap, every url is of the form http://www.oxygenboutique.com/<page_name>.aspx with little to no indication of whether a given page is about a product or something else. Filtering on categories names as you do might even filter out some actual products. Using a SitemapSpider does a pretty good filtering by itself that you should only need to check if the page is actually a product in your parsing function.

Miscellaneous

  • What's with that yield item?
  • Why a dict for the stock statuses instead of a list of available options?
  • Why a gender field if it's going to be constant?
  • What's with these empty line right after : (defs, ifs…)?
  • What's with all these float to str to float conversions to get prices and discounts?

Draft for improvement

# -*- coding: utf-8 -*-
import scrapy
import json
from pyquery import PyQuery
import urlparse
import urllib2

PRODUCTS_TYPES = {
    'S': {
        'HEEL', 'SNEAKER', 'SNEAKERS',
        'BOOT', 'FLATS', 'WEDGES',
        'SANDALS'
    },
    'J': {
        'RING', 'NECKLACE', 'RING',
        'BANGLE', 'CHOKER', 'COLLIER',
        'BRACELET', 'TATTOO', 'EAR JACKET'
    },
    'B': {
        'BAG', 'PURSE', 'CLUTCH',
        'TOTE'
    },
    'A': {
        'PINNI', 'BLOUSE', 'TOP',
        'SKIRT', 'KNICKER', 'DRESS',
        'DENIM', 'COAT', 'JACKET',
        'SWEATER', 'JUMPER', 'SHIRT',
        'SKINNY', 'SHORT', 'TEE',
        'PANTS', 'JUMPSUIT', 'HIGH NECK',
        'GOWN', 'TROUSER', 'ROBE',
        'PLAYSUIT', 'CULOTTE', 'JODPHUR',
        'PANTALON', 'FLARE', 'CARDIGAN',
        'VEST', 'CAMI', 'BEDSHORT',
        'PYJAMA', 'BRALET', 'TUNIC',
        'HOODY', 'SATEEN', 'BIKER',
        'JEAN', 'SWEAT', 'PULL',
        'BIKINI', 'LE GRAND GARCON'
    },
}

COLORS = {
    'BLACK', 'WHITE', 'BLUE',
    'YELLOW', 'ORANGE', 'GREY',
    'PINK', 'FUSCIA', 'RED',
    'GREEN', 'PURPLE', 'INDIGO',
    'VIOLET'
}


class OxygendemoItem(scrapy.Item):
    code = scrapy.Field()  # unique identifier (retailers perspective)
    description = scrapy.Field()  # Detailed description
    designer = scrapy.Field()  # manufacturer
    eur_price = scrapy.Field()  # full (non_discounted) price
    gender = scrapy.Field()  # F - Female, M - male
    gbp_price = scrapy.Field()  # full (non_discounted) price
    image_urls = scrapy.Field()  # list of urls representing the item
    link = scrapy.Field()  # url of product page
    name = scrapy.Field()  # short summary of the item
    raw_color = scrapy.Field()  # best guess of color. Default = None
    sale_discount = scrapy.Field()  # % discount for sale item where applicable
    stock_status = scrapy.Field()  # list of sizes in stock
    type = scrapy.Field()  # 'A' = apparel, 'B' = bags, 'S' = shoes, 'J' = jewelry, 'R' = accessories
    usd_price = scrapy.Field()  # full (non_discounted) price


class OxygenSpider(scrapy.spiders.sitemap.SitemapSpider):
    allowed_domains = ["oxygenboutique.com"]
    sitemap_urls = ['http://www.oxygenboutique.com/sitemap.xml']
    rules = (scrapy.spiders.Rule(
        # Extract links in sitemap but not matching 'products.aspx', 'contactus.aspx'…
        scrapy.linkextractors.LinkExtractor(deny=(
            'Products\.aspx',
            'ContactUs\.aspx',
            'SearchResult\.aspx',
        )),
        callback='parse_product',
    ),)

    def __init__(self, base_currency='GBP'):
        self.ex_rates = get_exchange_rates(base_currency)
        super(OxygenSpider, self).__init__()

    def parse_product(self, response):
        self.logger.info('Entered into parse_product method')
        self.logger.info('Received response from: {}'.format(response.url))
        self.logger.debug('Respons status: {}'.format(response.status))

        item = OxygendemoItem()
        DOM = PyQuery(response.body)
        # find that "add to shopping bag button" or we are not on a product page
        if not DOM.find('.btn_add_to_shopping_bag'):
            return
        base_url , item['code'] = parse_url(response.url)
        description = DOM('.right div#accordion').children().eq(
            1).text().encode('ascii', 'ignore')
        summary_bag = set(description.upper().split())
        item['description'] = description
        item['type'] = self.type_of_product(summary_bag)
        item['link'] = response.url
        item['name'] = DOM('.right h2').text()

        item['gbp_price'], item['sale_discount'] = price_and_discount(
            DOM('.price').children())

        if self.ex_rates is not None:
            base_price = item['gbp_price']
            for currency in ('USD','EUR'):
                field = '{}_price'.format(currency.lower())
                price = base_price * self.ex_rates[currency]
                item[field] = '{0:.2f}'.format(price)
        else:
            item['usd_price'], item['eur_price'] = ['N/A'] * 2

        item['designer'] = DOM('.right').find('.brand_name a').text()
        item['stock_status'] = [
            elem.text()
            for elem in DOM('select').children()
            if 'Sold Out' not in elem.text()]
        item['gender'] = 'F'  # Oxygen boutique carries Womens's clothing
        item['image_urls'] = [
            urlparse.urljoin(base_url, image.attrib['src'])
            for image in DOM('div#product-images tr td a img')]
        item['raw_color'] = self.color_of_product(summary_bag)

        return item

    @staticmethod
    def type_of_product(summary):
        for category, items in PRODUCTS_TYPES.iteritems():
            for item in items:
                if item in summary:
                    return category
        return 'R'  # Tag as accessory as failsafe

    @staticmethod
    def color_of_product(summary):
        ''' Will go this route to avoid external imports'''        
        try:
            return (summary & COLORS).pop()
        except KeyError:
            return None

    @staticmethod
    def price_and_discount(gbp_price):
        if not gbp_price('.mark').text():  # No discount
            discount = '0%'
            price = float(gbp_price.parent().text())
        else:  # Calculate discount
            price = float(gbp_price('.mark').text())
            new_price = float(gbp_price.eq(1).text())
            discount = '{0:.2f}%'.format(price / new_price * 100)
        return price, discount


def parse_url(raw_url):
    url = urlparse.urlparse(raw_url)
    page_name = url.path.lstrip('/').rsplit('.',1)[0]
    return '{}://{}'.format(url.scheme, url.netloc), page_name

def get_exchange_rates(base_currency):
    '''return a dictionary of exchange rates compared to the base currency'''
    url = 'http://api.fixer.io/latest?base={}'.format(base_currency)
    try:
        response = urllib2.urlopen(url)
        json_content = json.load(response.read())
        response.close()
        return json_content['rates']
    except urllib2.URLError:
        return None
| improve this answer | |
\$\endgroup\$
  • \$\begingroup\$ Wow thank you @Mathias. I guess I still have a lot to learn. Never seen @staticmethod decorators before. Also your other points mentioned in your answer make sense. \$\endgroup\$ – trendsetter37 Oct 24 '15 at 0:32
  • \$\begingroup\$ Also in regards to checking the page type (product or category) I tried to determine page type on the front end before it is passed through to the parse function. One of their grading parameters dealt with the ratio of scrapy.Requests to parsed items. I was under the impression that catching it before hand, i.e. using the regex for the sitemap url rules, would help cut down on initial Requests. I am not sure that I actually accomplished what was intended though. Realistically I managed to scrape 438 products out of 490 requests. \$\endgroup\$ – trendsetter37 Oct 24 '15 at 0:48
  • \$\begingroup\$ @trendsetter37 That makes sense. I added url rules to filter out pages that obviously are not products. You can extend it at will to get as close as 100% products requests ratio as possible. \$\endgroup\$ – 301_Moved_Permanently Oct 24 '15 at 8:03
  • \$\begingroup\$ @trendsetter37 Also I have the feeling that the PRODUCTS_TYPES collection should be built out of a query to avoid the need of maintaining it when the site adds new categories for instance… \$\endgroup\$ – 301_Moved_Permanently Oct 24 '15 at 8:10

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.