5
\$\begingroup\$

I'm getting familiar with python and I'm still learning it's tricks and idioms.

Is there an better way to implement print_html() without the multiple calls to html_file.write()? I only separated the HTML for clarity instead of putting the entire string inside write().

In get_car(), is there a way to avoid using if/else to make the dict key ' ' if listing.find() returns nothing?

Is there anything that is flat out wrong and should be avoided/improved?

#!/usr/bin/python 

import requests
import bs4
import os

### Declarations

home         = os.environ['HOME']
filename     = home + '/results.html'
results_file = open(filename, 'w')

req = requests.get([url_removed])

soup     = bs4.BeautifulSoup(req.content)
listings = soup.find_all('div', class_='listing listing-findcar listing-dealer     ')
listings.reverse()

### Functions

def print_header(message, html_file):
    html_file.write(message)


def print_html(car, html_file):
    price    = '<h3>' + car['price'].strip() + '</h3>'
    image    = '<img src="' + car['image'].strip() + '">'
    year     = '<li>' + car['year'].strip() + ' ' + car['trim'].strip() + '</li>'
    color    = '<li>' + car['color'].strip() + '</li>'
    miles    = '<li>' + car['miles'].strip() + '</li>'
    dealer   = '<li>' + car['dealer'].strip() + '</li>'
    phone    = '<li>' + car['phone'].strip() + '</li>'
    distance = '<li>' + car['distance'].strip() + '</li>'
    link     = '<li><a href="[url_removed]' + car['link'].strip() + '">[text_removed]</a></li>'

    html_file.write(price)
    html_file.write(image)
    html_file.write('<ul>')
    html_file.write(year)
    html_file.write(color)
    html_file.write(miles)
    html_file.write(dealer)
    html_file.write(phone)
    html_file.write(distance)
    html_file.write(link)
    html_file.write('</ul>')


def get_car(listing):
    car = dict()

    # Year
    if listing.find('span', class_='atcui-truncate ymm'):
        car['year'] = listing.find('span', class_='atcui-truncate ymm').get_text()
    else:
        car['year'] = ''

    # Trim
    if listing.find('span', class_='trim'):
        car['trim'] = listing.find('span', class_='trim').get_text()
    else:
        car['trim'] = ''

    # Price
    if listing.find('h4', class_='primary-price'):
        car['price'] = listing.find('h4', class_='primary-price').get_text()
    else:
        car['price'] = ''

    # Mileage
    if listing.find('span', class_='mileage'):
        car['miles'] = listing.find('span', class_='mileage').get_text()
    else:
        car['miles'] = ''

    # Color
    if listing.find('span', class_='color'):
        car['color'] = listing.find('span', class_='color').get_text()
    else:
        car['color'] = ''

    # Distance
    if listing.find('span', class_='distance-cont'):
        car['distance'] = listing.find('span', class_='distance-cont').get_text()
    else:
        car['distance'] = ''

    # Dealer
    if listing.find('span', class_='owner-title atcui-block atcui-bold'):
        car['dealer'] = listing.find('span', class_='owner-title atcui-block atcui-bold').get_text()
    else:
        car['dealer'] = ''

    # Dealer phone
    if listing.find('span', class_='owner-phone'):
        car['phone'] = listing.find('span', class_='owner-phone').get_text()
    else:
        car['phone'] = ''

    # Image
    if listing.find('img', class_='atcui-vehicle-image   '):
        car['image'] = listing.find('img', class_='atcui-vehicle-image').get('src')
    else:
        car['image'] = ''

    # Link
    if listing.find('a', class_='vehicle-title atcui-trigger '):
        car['link'] = listing.find('a', class_='vehicle-title atcui-trigger ').get('href')
    else:
        car['link'] = ''

    return car

### Main

print_header('<h3>' + str(len(listings)) + ' CARS FOUND</h3>', results_file)

for listing in listings:
    listing_dict = get_car(listing)
    print_html(listing_dict, results_file)
\$\endgroup\$

3 Answers 3

5
\$\begingroup\$

You can make the HTML a multi-line string with formatting templates (see this SO question for discussion on indentation, I've kept it simple):

def print_html(car, html_file):
    html = '''<h3>{0[price]}</h3>
<img src="{0[image]}">
<ul>
     <li>{0[year]} {0[trim]}</li>
     ...
</ul>'''

Then your writing becomes a single line:

    html_file.write(html.format(car))

Note that this doesn't strip; you should do that when the data go into the dictionary.


There is a lot of duplication in get_car. Also, you do the find twice, which will slow you down. Try something like:

def get_car(listing):
   car = dict()
   data = {'year': ('span', 'atcui-truncate ymm'),
           ...}

   for key in data:
       element, class_ = data[key]
       result = listing.find(element, class_=class_)
       car[key] = '' if result is None else result.get_text().strip()

   return car 

Finally, you aren't always following the style guide, e.g. lining up operators:

### Declarations

home = os.environ['HOME']
filename = home + '/results.html'
results_file = open(filename, 'w')
\$\endgroup\$
0
2
\$\begingroup\$

The problem that you are trying to solve — transforming one HTML document into another — is precisely the problem that solves. Consider using a domain-specific language that is built for the job, rather than . Even though XSLT is rather ugly, the solution would be much more compact.

import lxml.etree as ET

transform = ET.XSLT(ET.fromstring('''<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">
  <xsl:output method="html" indent="yes" />

  <xsl:template match="//*[div[@class='listing listing-findcar listing-dealer']]">
    <body>
      <h3><xsl:value-of select="count(*)"/> CARS FOUND</h3>
      <xsl:apply-templates select="*">
        <!-- Display results in reverse -->
        <xsl:sort select="position()" data-type="number" order="descending"/>
      </xsl:apply-templates>
    </body>
  </xsl:template>

  <xsl:template match="//div[@class='listing listing-findcar listing-dealer']">
    <div>
      <h3><xsl:value-of select=".//h4[@class='primary-price']"/></h3>
      <img src="{img[normalize-space(@class)='atcui-vehicle-image']/@src}"/>
      <ul>
        <li><xsl:value-of select=".//span[normalize-space(@class)='atcui-truncate ymm']"/> <xsl:value-of select=".//span[@class='trim']"/></li>
        <li><xsl:value-of select=".//span[@class='color']"/></li>
        <li><xsl:value-of select=".//span[@class='mileage']"/></li>
        <li><xsl:value-of select=".//span[@class='owner-title atcui-block atcui-bold']"/></li>
        <li><xsl:value-of select=".//span[@class='owner-phone']"/></li>
        <li><xsl:value-of select=".//span[@class='distance-cont']"/></li>
        <li><a href="{.//a[normalize-space(@class)='vehicle-title atcui-trigger']/@href}">[text removed]</a></li>
      </ul>
    </div>
  </xsl:template>
</xsl:stylesheet>
'''))

req = …
input = ET.fromstring(req.content)
output = transform(input)
with open(…, 'w') as results_file:
    results_file.write(ET.tostring(output))
\$\endgroup\$
1
  • \$\begingroup\$ XSLT looks very intriguing. I'll look at the docs and give it a spin. Thanks \$\endgroup\$
    – Creek
    Aug 23, 2014 at 2:45
2
\$\begingroup\$

Constructing HTML output by concatenating '<tag>' + text + '</tag>' is a bad idea, not just for aesthetic reasons. You could be vulnerable to cross-site scripting problems (also known as XSS or HTML injection). For example, if car['miles'] = '<80k', then the output would contain

<li><80k</li>

… which is malformed HTML.

One alternative approach is to use an HTML templating library. In this case, however, your existing code seems like it would be better written using a tag-building library. One such library is ElementTree, which is a standard Python package. An alternative would be markup.py.

import xml.etree.ElementTree as ET

def to_html(car):
    def child(parent, tag, *car_text, **car_attrs):
        e = ET.SubElement(parent, tag)
        e.text = ' '.join([car.get(t, '').strip() for t in car_text])
        for attr, car_attr in car_attrs.iteritems():
            if car_attr in car:
                e.set(attr, car[car_attr].strip())
        return e

    container = ET.Element('div')
    child(container, 'h3', 'price')
    child(container, 'img', src='image')
    ul = child(container, 'ul')
    child(ul, 'li', 'year', 'trim')
    child(ul, 'li', 'color')
    child(ul, 'li', 'miles')
    child(ul, 'li', 'dealer')
    child(ul, 'li', 'phone')
    child(ul, 'li', 'distance')
    link = child(ul, 'li')
    child(link, 'a', 'link_text', href='link')

    return ''.join(ET.tostring(child) for child in container)

def print_html(car, html_file):
    html_file.write(to_html(car))

As I've written it, the child() helper function is tolerant of unset attributes, which relieves get_car() of the duty to set unknown values to ''.

\$\endgroup\$
1
  • \$\begingroup\$ I didn't even think about the security implications of this, excellent point. I notice you return list comprehensions, I'm going to start doing that. \$\endgroup\$
    – Creek
    Aug 23, 2014 at 2:30

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.