Return to Revisions

• Do not use capitalization for function or variable 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. With 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)
• 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):
• Use sub functions to break up your code to improve readability and decrease complexity. Splitting up your code into logical pieces also improves testability and reusability. This function is on the larger side.

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 a 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

You could still randomly pick a filename, however I would still check if that filename already exists to stop overwriting. When you pick random numbers there is some chance that a number may appear in the same program run. A program is not robust if it steps on its own toes by potentially overwriting files it just downloaded.

You could use this generator in the main function:

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 but so does the pluralization links. 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. Also indexing the first element of each list in links_list (from your code) belongs here, since it's an implementation detail of finding the links from an html source tree.

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))

Note: I did not do error checking and exception handling, since the other answer covers that appropriately. I think the best solution would be some combination of mine and @MrGrj's answer.

Also make sure your imports are at the top like @MrGrj's in the actual document.