2
\$\begingroup\$

When I am scraping from a web page I need a way to open and read it. Previously, I used two functions, one for requests and one for Selenium

def read_page_w_requests(url):
    try:
        headers = {'User-agent': 'Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) \
                    Gecko/2008071615 Fedora/3.0.1-1.fc9 Firefox/3.0.1'}
        r = requests.get(url, headers=headers)
        text = r.text
    except Exception, e:
        print "Read page with requests error:", e
        text = None
    return text


def read_page_w_selenium(driver, url, element_be_located=""):
    try:
        driver.get(url)
        wait = WebDriverWait(driver, 10)
        if element_be_located:
            wait.until(EC.visibility_of_element_located((By.ID, element_be_located)))
            text = driver.page_source
        else:
            text = driver.page_source

    except Exception, e:
        print "Read page error:", e
        text = None

    return text

I am thinking of using just one, that combines both:

import requests
from selenium import webdriver
from selenium.webdriver.support.wait import WebDriverWait
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC


def get_driver():
    try:
        dcap = dict(DesiredCapabilities.PHANTOMJS)
        dcap["phantomjs.page.settings.userAgent"] = (
            "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/53 "
            "(KHTML, like Gecko) Chrome/15.0.87"
        )
        driver = webdriver.PhantomJS(desired_capabilities=dcap)    
        return driver
    except Exception, e:
        print "Driver error:", e
        return None

def get_page_source(url, driver="", element=""):
    if driver:
        try:
            driver.get(url)
            wait = WebDriverWait(driver, 10)
            if element:
                wait.until(EC.visibility_of_element_located((By.ID, element)))
            text = driver.page_source
        except Exception, e:
            print "Read page error:", e
            text = None

    else:    
        try:
            headers = {'User-agent': 'Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) \
                        Gecko/2008071615 Fedora/3.0.1-1.fc9 Firefox/3.0.1'}
            r = requests.get(url, headers=headers)
            text = r.text
        except Exception, e:
            print "Read page error:", e
            text = None

    return text

Is it a good idea to combine two functions that do the same thing with a different way in a single function? One of my concerns is the name of each function.

\$\endgroup\$
  • \$\begingroup\$ Curious as to your use case - why are you scraping data from a web page? Are you sure the provider does not have an API? \$\endgroup\$ – Dan Pantry Oct 12 '15 at 9:46
  • \$\begingroup\$ If there is an API, I use it. Otherwise, I use requests and Selenium. \$\endgroup\$ – GiannisIordanou Oct 12 '15 at 9:53
  • \$\begingroup\$ Side point: use except SomeException as exc, not the version with a comma. This has long been the current standard. \$\endgroup\$ – user86624 Oct 12 '15 at 9:57
  • \$\begingroup\$ @holroy I added the get_driver function. Is there anything else wrong ? \$\endgroup\$ – GiannisIordanou Oct 12 '15 at 10:01
  • \$\begingroup\$ That's what I get for not making up my mind about the name of a parameter. \$\endgroup\$ – GiannisIordanou Oct 12 '15 at 10:36
3
\$\begingroup\$

I don't think it's worth having as one function. Why? Because you don't get to share any of the functionality. Your singular wrapped function just tests if there's a driver value and will execute one of two completely separate code blocks as a result. Instead, consider having the two functions separate, and then a single wrapper function that you can call which will determine which one to call.

In other words:

def get_page_source(url, driver="", element=""):
    if driver:
        return read_page_w_selenium(driver, url, element)
    else:    
        return read_page_w_requests(url)

This way you only need to call get_page_source but you're not cramming two unrelated blocks into the same function.

Other notes, you shouldn't set driver as an empty string if you expect an object. Set the default value as None and then test for that.

def get_page_source(url, driver=None, element=""):
    if driver is None:
        return read_page_w_selenium(driver, url, element)

Also it's confusing to change the order of arguments. Make url first in both functions so that the order is consistent.

Instead of using text = None then return text. You could use a premature return and return None, though in Python that's equivalent to return. But I think even better would be the opposite, just use a premature return when you successfully get the text:

def read_page_w_requests(url):
    try:
        headers = {'User-agent': 'Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) \
                    Gecko/2008071615 Fedora/3.0.1-1.fc9 Firefox/3.0.1'}
        r = requests.get(url, headers=headers)
        return r.text
    except Exception, e:
        print "Read page with requests error:", e

Now your exception doesn't even need to return None, because by default a Python function without an explicit return will just return None anyway.

\$\endgroup\$
  • \$\begingroup\$ Your remarks about the order of arguments and the premature return are very useful. My only motivation in using a single function was that read_page_w_requests doesn't seem like a suitable function name. \$\endgroup\$ – GiannisIordanou Oct 12 '15 at 10:38
  • \$\begingroup\$ @evil_inside I do agree with that. If I knew more about reading web pages I might be able to help. Perhaps something like requests_page or page_requests would be more appropriate? I definitely think _w isn't necessary, and you don't need to explicitly have read_page when the terms are common to both functions. \$\endgroup\$ – SuperBiasedMan Oct 12 '15 at 10:51
0
\$\begingroup\$

Style review

  • Try to keep your line shorter than 80 chars. It's not a requirement, more like a common python convention. Whether fixed-length lines is a good idea or not is debatable, but if you want to interact with others in the python community it's usually better to follow their conventions.
  • Be more expressive with your variable names. read_page_w_selenium should've been read_page_with_selenium. r and dcap aren't always the most expressive of names. (For the record, I believe that finding good variable names is a difficult exercise and it improves the overall structure of your program).
  • return None at the end of a function isn't necessary. In Python, a function without explicit return value, will return None.
  • You don't need a backslash (\) for line returns inside braces (headers = {'User-agent': 'Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) \)
  • You want to use the syntax except XException as e:. It's more portable and will nest multiple exceptions in a more elegant way.
  • Not exactly a style comment, but avoid as much as possible except Exception, e: statements. We call this a Pokemon exception (because, you know... gotta catch'em all). When you're catching an exception, make sure to specify which and let the others leak up so that you get a better feel for the errors in your program.
  • Your code is valid in python2. It would take very little effort to make it valid in both python2 and python3. It's a bad idea to ignore python3.
  • You're using empty strings for default arguments when you should probably be using None. (Some would argue that default arguments should be avoided altogether).

Is it a good idea to combine two functions that do the same thing with a different way in a single function?

In my opinion it almost always is.

There are really 2 things you want to take into account here:

  • Make the code more organized

You want to avoid code duplication as much as possible. Reduces the risk of human errors, and makes future refactoring easier.

  • Make the code easier to use

Make it easier to your user to start using your function. This has to do with API design, and I believe you have multiple choices happening here. Do you want the user to pick whether to use request or not? Or should your program know exactly what module to use? Are you presenting 2 separate functions in your API or just the one? What are the arguments the user should provide?

There's no correct answers to these questions, just a lot of choices you have to be aware of when designing your functions/API.

Refactoring

The goal of refactoring was to limit code duplication. Why not go for something a bit more efficient?

def get_page_source(url, driver=None, element=None):
    try:
        if driver:
            driver.get(url)
            wait = WebDriverWait(driver, 10)
            if element:
                wait.until(EC.visibility_of_element_located(
                    (By.ID, element)))
            text = driver.page_source
        else:
            moz_user_agent = \
                "Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) " \
                "Gecko/2008071615 Fedora/3.0.1-1.fc9 Firefox/3.0.1'}"
            headers = {'User-agent': moz_user_agent}
            request = requests.get(url, headers=headers)
            text = request.text
    except Exception as e:
        print "Read page error:", e
        text = None

    return text

I still believe this should be improved:

  • the parameters driver and element don't make sense for the request function. I would probably go for something like get_page_source (url, **args), and then check for if 'driver' in **args:.
  • the function get_driver could be incorporated into that function (or not, both are valid choices)
  • It really needs better error/exception handling.
\$\endgroup\$
0
\$\begingroup\$

Do read other answers as I will only comment on two subjects: Choice of combining functions and Exception catching. At end is a little untested code suggestion.

Choice of combining functions

Whether to combine two functions into one or not, depends on a multitude of factors, so a general answer is hard to give. However some pointers can be given.

A function should primarily have one and only one concern or task. And this task it should perform well. In this case the task is to get the source of an url, which is a well defined task. However, if this were part of a larger library of url handling, then that task could be unclear as maybe many functions have that concern or task.

A function could group related functions to form a better interface. It could be valid to do as you've done to have a common interface to getting URL's, but if you need to add loads of stuff to distinguish between them as parameter, then you are at a loss. I've seen code jamming many parameters into one function, and then having massive if's on differement parameters to distinguish which sub function to perform. That is not good.

Another advantage of having a grouping function, could be to have common error handling. In your code you could make the if driver within the try ... except and you could harvest those benefits. You could i.e. make it a loop trying different version for getting the url.

Exception catching

Related to error handling, it is generally considered bad advice to catch Exception. Catch the most specific exception available. Locate what exceptions your sub functions raises and catch these. Maybe they raise something like HttpError.

Also it is better to do stuff like except HttpError as error instead of using commas. The behaviour of using commas here both makes it unclear, and will be addressed/depreceated in newer versions of Python.

It is good advice to make unique exception messages so that it is easy to find again, even if your code project is several thousand or millions code lines. Using your good function name could be one way of making it unique. Remember that in the large project you never know where the exception might occur, so it's good being able to locate it.

Another tidbit is to include vital parameters in exception message. Yet again, if your code base is large and you don't know where the exception is triggered it is wise to include enough information so that you are able to regenerate the exception in your test environment. Just having an "get_page_source failed" doesn't help anyone, but "get_page_source failed - url: Httttttttp::////does.it.fail/" clearly indicates why it failed.

Code suggestion

Maybe something like the following untested code would be my take on it:

def get_page_source(url, driver="", element=""):
    """Get source of page at url, or return None"""

    try:
        # Try selenium driver ...
        if driver:
            driver.get(url)
            wait = WebDriverWait(driver, 10)
            if element:
                wait.until(EC.visibility_of_element_located((By.ID, element)))
            return driver.page_source

        # or use basic requests
        else:    
            headers = {'User-agent': 'Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.1) \
                        Gecko/2008071615 Fedora/3.0.1-1.fc9 Firefox/3.0.1'}
            request = requests.get(url, headers=headers)
            return request.text

    except HttpError as error:
        print("get_page_source failed - url=url,  error=".format(url, error))
\$\endgroup\$

Not the answer you're looking for? Browse other questions tagged or ask your own question.