Return to Revisions

• Do not use capitalization for function name this violates idiomatic python naming conventions (PEP8 link). Try out pylint to catch these kinds of issues
• Prefer str.format() over concatenation. str.format() is not only a lot more flexible, but easier to use than concatenation and more efficient for combining more than 2 strings. str.format() you do not need to explicitly convert objects to the string type. (less TypeError exceptions) Instead of str(name) + ".jpg" do "{}.jpg".format(name)
• Use sub functions to break up your code. This function is on the larger side.
• Prefer iteration over indexing. A list or any container can be iterated over to yield its contents. For example do for link in links_list: instead of while k <= (len(links_list) - 1):

Randomly picking a filename is decidedly not a good idea. The given filename may already exist, and urlretrieve will go ahead and overwrite the file.

Here's would be an example function for retrieving a unique filename.

def unique_filename():
    """Yield a unique filename.

    The given filenames start at 0.jpg, 1.jpg and so on
    """
    for filenum in count():
        filepath = "{}.jpg".format(filenum)
        if not os.path.exists(filepath):
            yield filepath

then you could do something like:

for link, filename in zip(links_list, unique_filename()):
    urllib.urlretrieve(link, filename)

We use a generator to yield only filenames that do not exist, and use the finite links_list to limit the generator since zip stops when the first iterable is exhausted.

As a side note, links_list is a very redundant name. A list implies multiple. I (and Brandon Rhodes) would rather name it link_list. If this was ruby, you would want to just name it links.

Lets pull out the link finding code into its own function.

def image_links(soup):
    """Yield a src tag links from given BeautifulSoup object."""
    for link in soup.findAll('img'):
        image_links = link.get('src')
        if '.jpg' in image_links:
            for i in image_links.split("\\n"):
                yield i.split()[0]

I do have a suspicion that some of these splits are not needed.

Bringing these two functions together we get an easier to understand main function:

def download_images_from_page(url):
    """Download jpg images from html at given url to the local filesystem.

    The use the cwd for the location of the downloaded images.
    """
    source_code = requests.get(url)
    plain_text = source_code.text
    soup = BeautifulSoup(plain_text, "html.parser")
    for link, filename in zip(image_links(soup), unique_filename()):
        urllib.request.urlretrieve(link, filename)
        print(link)
        print("{}\n".format(filename))