* Do not use capitalization for function names. This violates idiomatic python naming conventions ([PEP8 link](https://www.python.org/dev/peps/pep-0008/#function-names)). Try out [pylint](https://www.pylint.org/) 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](https://www.youtube.com/watch?v=YklKUuDpX5c)) 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))