5
\$\begingroup\$

What are your thoughts and comments relating to areas I could improve on?

import csv
import re
import requests
from bs4 import BeautifulSoup


class Resto:
    def __init__(self,url,filename="default"):
        self.page = 1
        url = url + "&page={0}"
        self.url = url 
        self.filename = filename

    def info(self):
        linkz = []
        while True:
            content = requests.get(self.url.format(self.page)).content
            soup = BeautifulSoup(content)
            links = [i["href"] for i in soup.find_all("a",{"itemprop":"name"})]
            if not links:
                break
            linkz.extend(links)
            self.page += 1
        return linkz

    def email(self):
        names = []
        emails = []
        sites = []
        streets = []
        cities = []
        numbers = []
        for i in self.info():
            i = "http://www.yellowpages.com" + i
            content2 = requests.get(i).content
            soup2 = BeautifulSoup(content2)
            name = [i.text for i in soup2.find_all("h1",{"itemprop":"name"})]
            email = [re.sub(r'mailto:', "",i["href"]) for i in soup2.find_all("a",{"class":"email-business"})]
            if email:
                email = [email[0]]
            site = [i["href"] for i in soup2.find_all("a",{"class":"custom-link"}) if i.text == "Visit Website"]
            street = [i.text for i in soup2.find_all("p",{"class":"street-address"})]
            city = [i.text for i in soup2.find_all("p",{"class":"city-state"})]
            phone = [i.text for i in soup2.find_all("p",{"class":"phone"})]

            if not email:
                email = ["---"]
            if not site:
                site = ["---"]

            if not street:
                street = ["---"]
            if not city:
                city = ["---"]
            if not phone:
                phone = ["---"]

            names.extend(name)
            emails.extend(email)
            sites.extend(site)
            streets.extend(street)
            cities.extend(city)
            numbers.extend(phone)
        return names,streets,cities,numbers,sites,emails

    def csv(self):
        """saves csv file in root of python 3 directory"""
        items = self.email()
        items = list(zip(*items))
        with open("{0}.csv".format(self.filename), "w")as shops:
            writer = csv.writer(shops)
            writer.writerows(items)


a = Resto("http://www.yellowpages.com/search?search_terms=restaurants&geo_location_terms=tx")
a.csv()
\$\endgroup\$
5
\$\begingroup\$

You have an awful lot of excess whitespace. Use one or two blank lines between functions, and one blank line to break up different parts of each function (especially, just below the end of a loop or if block). Don't put blank lines immediately between a line ending in : and the start of the indented block beneath it.

The while loop in info uses a class attribute as a loop counter, which actually seems to cause a potential bug here: the counter persists, so on subsequent calls you will skip the loop, never append to the outer list, and it will be returned empty. If page is a local variable instead, and it will work fine no matter how many calls you make.

Instead of keeping a loop counter like this, and incrementing it manually, preferred Python style is to use a for loop. The equivalent for loop to your current while loop would be:

for page in range(1, 2):
   ...

Although it would be good to store the maximum page in a variable. But in light of your clarification in the comments that this would normally be a while True: loop, which only breaks once it hits a page with no interesting links, then the equivalent is to use itertools from the stdlib:

for page in it.count(1):
    ...

You have a variable links to capture the links on each iteration of the loop, and linkz to accumulate them across all the pages you parse (which is only one at the moment anyway, but I'm assuming that could change later). Those names are quite confusing; it would be good to differentiate better - call them links for the one that gathers all the links, and page_links for the one that gets the links for just this page.

Your email function parses what I would call your 'main' data structure. Since that is a lot more than just email-related information, email isn't the best name for it.

The data structure that email creates seems awkward for your data. If you think of people as 'records', and the information you capture about them to be 'fields', then it would make more sense to have it as a list of namedtuples, or or even a pandas dataframe if you're working with a lot of data. That way, your code to parse it is a little bit simpler:

person = namedtuple(...)
people = []
for link in links:
   this_person = person()
   person.phone = #parse phone number
   etc.

return people

But this is the major data structure in your class. It contains all the information that this class is designed to parse. So, I think all of this should be done in __init__, with this structure - and only this structure - captured as an attribute.

The only other attribute you store is the filename that gets passed to __init__. I think it would make more sense as an argument to your csv method - the only method that uses it - because the filename to write it to is a decision that goes with "I want to write this data to disk" more than "I want to scrape some data from a website".

So, you're left with a class with one attribute, and two methods - one of which is __init__, and the other is fairly trivial. In fact, csv becomes even more trivial with a list of namedtuples, since you wouldn't need to zip it. Similarly if you use a pandas DataFrame, it has it's own CSV routines that you can use.

That means there is no need for a class. Instead, use what your __init__ has grown to as a standalone function - call it parse_yellowpages(url). Have it return the list or DataFrame. Then drop the csv function entirely and just put that in the main line of your program.


Your current code is a little schizophrenic about whether it allows multi-valued fields: eg, can a person have more than one phone number?

The code parsing it looks like it does. You parse out a list of data, then you extend it onto the accumulated list. But then when you write out your CSV, you assume (by your use of zip to regroup them) that all fields have exactly one value.

In the comments you clarified that you only want single-valued fields. That being the case, don't include a full list of every match you found in your data structure. If you get three phone numbers for a person when you're only expecting one, you want to either consider it an error and bail out, or forget all but one of them. Your current code would silently remember all of them, forget that it did that, and end up with corrupt data.

To take the 'signal an error' path, do something like this:

phone_numbers = soup2.findall(...)
if len(phone_numbers) != 1:
    raise RuntimeError('Empty or multivalued field: ' + url)
else:
   this_person.phone = phone_numbers[0]

If you prefer to instead ignore any surplus values, use find instead of find_all - they take the same arguments, but find will just return one result instead of a list of them.

\$\endgroup\$
  • \$\begingroup\$ list isn't remembered but extends an empty list outside of the loop before the next iteration.so the values are passed and therefore the values are remembered. Completely fail to correlate the names and emails and numbers to appropriate people? wut? at first glance that might be. but the zip joined back all those values together because they were extracted on the same page and extended into a list in the same order so zipping them fixed that. As for everything else you said besides those points. I completely agree with you. whitespace, variable naming, and class use. and thank you \$\endgroup\$ – Halcyon Abraham Ramirez Jul 29 '15 at 19:55
  • \$\begingroup\$ The outer list isn't remembered. The second time that function is called, self.page is already 2, so the while loop is skipped, and it returns an empty list. For associating fields with the right records, storing them in the same order works if there's the same number of every field for every person - but the rest of your code suggests that that doesn't have to be true. If one person has three phone numbers (say, home, work and mobile) and two email addresses, those associations will break. \$\endgroup\$ – lvc Jul 30 '15 at 0:00
  • \$\begingroup\$ If you want to support multi-valued fields, your data structure should be set up to explicitly support that they are multivalued. If you only want to support single valued fields, your code should behave appropriately by storing only the zeroth value that is scraped. It is always good practice to be prepared for malformed data, and either clean it up if you can do that sensibly, or error out early. As written, your code will break silently and subtly. Pick a rule and stick to it. Storing records instead of lists of fields helps with that. \$\endgroup\$ – lvc Jul 30 '15 at 0:08
  • \$\begingroup\$ I get what you mean. but I explicitly tell it to stop at page two for the sake of bailing out the loop early if I wanted it i'll put it at while True so it'll scrape everything. but I just wanted one page for the tests sake. the outer list is remember it other wise if it weren't remembered I would have had an empty output... while I agree that the associations may break if there is more than one value in those fields but going through yellow pages there will always just be one value one address one name one number for the selected xpath that it's in. except for the email \$\endgroup\$ – Halcyon Abraham Ramirez Jul 30 '15 at 2:50
  • \$\begingroup\$ I did prepare for malformed data hence the line if not email \$\endgroup\$ – Halcyon Abraham Ramirez Jul 30 '15 at 2:51
1
\$\begingroup\$

Some of extra notes about the code:

  • when using BeautifulSoup it is strongly encouraged to specify an underlying parser explicitly. Otherwise, BeautifulSoup would pick the one automatically and the choice may differ from machine to machine:

    soup = BeautifulSoup(content, "html.parser")
    # or soup = BeautifulSoup(content, "html5lib")
    # or soup = BeautifulSoup(content, "lxml")
    
  • since you are issuing multiple requests to the same the host, consider creating a web-scraping Session() instance and reuse - this may result into a performance boost:

    if you're making several requests to the same host, the underlying TCP connection will be reused, which can result in a significant performance increase

  • find_all() calls in your case can be replaced with more concise CSS selector select() calls. For example, soup2.find_all("a",{"class":"custom-link"}) would be soup2.select("a.custom-link")

\$\endgroup\$

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.