Return to Revisions

• Do not use capitalization for function names. 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.

from itertools import count
import os


def unique_filename(base_dir="", fail_cond=os.path.exists):
    """Yield a unique filename.

    Args:
        base_dir:
            Directory path to yield filepaths from and check
            for existing files. Should already be created.
        fail_cond:
            Function to call to check if the given filename
            should be yielded. Must return a bool value.
            If the function returns false for the given filename
            then the given filename will be yielded

    The given filenames start at 0.jpg, 1.jpg and so on
    """
    pathfmt = os.path.join(base_dir, "{}.jpg")
    for filenum in count():
        filepath = pathfmt.format(filenum)
        if not fail_cond(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 src tag jpg urls 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.

We can also pull out the html tree object creation code into its own function:

def get_soup(url):
    """Get BeautifulSoup object from given url.

    Does not do any error checking.
    """
    source_code = requests.get(url)
    plain_text = source_code.text
    return BeautifulSoup(plain_text, "html.parser")

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

from bs4 import BeautifulSoup
import requests
import urllib.request


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."""
    soup = get_soup(url)
    notify_fmt = "{url}\n{filename}\n"
    for link, filename in zip(image_links(soup), unique_filename()):
        urllib.request.urlretrieve(link, filename)
        print(notify_fmt.format(url=link, filename=filename))