Clean up repeated file.writes, if/elses when adding keys to a dict

Question

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)

Answer 1

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')

Answer 2

The problem that you are trying to solve — transforming one HTML document into another — is precisely the problem that xslt solves. Consider using a domain-specific language that is built for the job, rather than reinventing-the-wheel. 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))

Answer 3

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 ''.