Function for getting URLs

Question

This is a simple Python program I wrote. I hope some one could help me optimize this program. If you found any bad habits, please tell me.

#!/usr/bin/python

import urllib2

def GetURLs(srcURL=""):

    if srcURL == "":
        return -1

    Content = urllib2.urlopen(srcURL, None, 6)
    oneLine = Content.readline()
    while oneLine:
        print oneLine
        oneLine = Content.readline()
    Content.close()
    return 0

GetURLs("http://www.baidu.com")

Answer 1

You asked for comments on coding style, so here goes:

• It's Python style to use CapitalizedNames for classes and lower_case_names for functions, methods and variables.

• Functions should have docstrings that describe their arguments and what they do.

• Names starting with "get" are normally used for functions that return the value they get. Your function prints the content at the URL, so it would be better to call it something like print_url.

• The value of 6 for the timeout argument to urlopen seems arbitrary. It would be better either to leave this out (the caller can always use socket.setdefaulttimeout if they need to set a timeout), or to allow the caller to pass it as a keyword argument.

• You can pass the timeout argument as a keyword argument to avoid having to specify None for the data argument.

• Having a default value for srcURL is pointless, especially since it's an invalid value! Omit this.

• The return value is useless: it's -1 if the argument was an empty string, or 0 if it wasn't. If the caller needs to know this, they can look at the argument they are about to supply. (Or they can catch the ValueError that urlopen raises if passed an empty string.)

• The Content object does not get closed if the readline function raises an error. You can ensure that it gets closed by using Python's try: ... finally: ... statement. (Or contextlib.closing if you prefer.)

Applying all of these improvements, plus the one suggested by Lev, yields this function:

def print_url(url, timeout=6):
    """
    Print the content at the URL `url` (but time out after `timeout` seconds).
    """
    content = urllib2.urlopen(url, timeout=timeout)
    try:
        for line in content:
            print line
    finally:
        content.close()

Answer 2

You can shorten your code by optimizing iteration over the lines. urlopen() returns a file-like object that allows direct iteration:

content = urllib2.urlopen(srcURL, None, 6)
for line in content:
    print line

content.close()

Answer 3

This solution is a bit more idiomatic:

def getURLs(srcURL=""):
    if not srcURL:
        return -1
    content = urllib2.urlopen(srcURL, None, 6)
    try:
        for oneLine in content:
            print oneLine
    finally:
        content.close()
    return 0

Notice that by convention, function names start with lower-case. I simplified the test condition at the beginning, and more importantly, the iteration can be written in a simpler way. And as a good practice, resources should be closed in a finally block.