This code takes a website and downloads all .jpg images in the webpage. It supports only websites that have the img element and src contains a .jpg link. The previous version can be found here

import random

import urllib.request

import requests

from bs4 import BeautifulSoup

# variables
k = 0
# source code to BeatifulSoup
source_code = requests.get(url)
plain_text = source_code.text
soup = BeautifulSoup(plain_text, "html.parser")
# filtering

while k <= (len(links_list) - 1):
# create a random name
name = random.randrange(1, 1000)
fullName = str(name) + ".jpg"
urllib.request.urlretrieve(lines, fullName)

print(lines)
print(fullName + '\n')

k += 1
# choose a site that has the <img> element in its html source code with src tag

• Don't re-invent the wheel. Website scraping is a well-solved problem: wget --accept jpg,png http://example.com. Additional flags like recursive as you need. Note that neither wget nor your work-in-progress will get anything inserted into the page with javascript, and that's very common these days. – peter Apr 30 '17 at 23:58

There's a lot to improve about your code, and I'll start from the top. I'll also rewrite some of the logic you had there because IMO it's a bit clumsy and hard to understand.

### Imports

Don't use urllib and requests at the same time. Use either one or another. They serve the same purpose.

from bs4 import BeautifulSoup
import os
import random
import requests


As you can see, I've alphabetically ordered them so that it becomes easier to locate each of them.

### Constants

You have one url which might be moved at the top of your program. That way, you'll allow your users to change it without searching through all the code.

Moving on, I realised that you didn't give the user a chance to save the pictures wherever he wants. Let's add another constant, say IMAGE_LOCATION.

URL = 'https://pixabay.com'
IMAGE_LOCATION = '/home/images/'


### Functions

You can split your code into separate functions so that each of them can serve its own purpose. Read more about Single Responsability Principle:

def get_html_source():
"""Docstring here."""
html_source = requests.get(URL).text
return BeautifulSoup(html_source, 'html.parser')


The above function will return a BeautifulSoup object which contains the html source code of the desired URL.

We can now build another function which will generate a path containing a random name for each picture we're downloading:

def image_name():
"""Docstring here."""
return '{}.jpg'.format(os.path.join(IMAGE_LOCATION, str(random.randrange(1, 100000))))


I've also increased the maximum number just in case there's a huge number of files.

Another helper function would be one that checks if a src URL is a valid one or not. I've modified a bit your condition and made it more strict:

def validate(image_url):
"""Docstring here."""
return image_url.endswith('.jpg') and image_url.startswith('http')


The above, verifies if the url starts with http (implicitly https will pass) and also if it ends in .jpg.

Only now, we can finally create the download_images function, and wrap our logic into it:

def download_images(soup):
"""Docstring here."""
if validate(image_url):
image = requests.get(image_url)
if image.status_code == 200:
with open(image_name(), 'wb') as f:
f.write(image.content)


As you can see, I've removed most of the logic you had, and simply did the following steps:

1. Go through all the img tags and save their src attribute (which is an URL
2. Check the URL if it matches our condition and build our image object
3. Double check to see if the status code was successful (200) and save the image to our desired location.

Next, wrapping all our code within a main function is as simple as:

def main():
"""Docstring here."""
soup = get_html_source()


### All the code

from bs4 import BeautifulSoup
import os
import random
import requests

URL = 'https://pixabay.com'
IMAGE_LOCATION = '/home/images/'

def get_html_source():
"""Docstring here."""
html_source = requests.get(URL).text
return BeautifulSoup(html_source, 'html.parser')

def image_name():
"""Docstring here."""
return '{}.jpg'.format(os.path.join(IMAGE_LOCATION, str(random.randrange(1, 100000))))

def validate(image_url):
"""Docstring here."""
return image_url.endswith('.jpg') and image_url.startswith('http')

"""Docstring here."""
if validate(image_url):
image = requests.get(image_url)
if image.status_code == 200:
with open(image_name(), 'wb') as f:
f.write(image.content)

def main():
"""Docstring here."""
soup = get_html_source()

if __name__ == '__main__':
main()


If you don't know what's with if __name__ == '__main__' you can read more about it here

More, note that all the functions have a docstring which contain a placeholder like "Docstring here.". You can replace that placeholder with a summary that describes what each function does.

• Your otherwise good answer is kind of ruined by the """Docstring here.""" lines. Either write a proper docstring, or leave it out. As it stands it is a screaming reminder of something to do, which isn't done. – holroy Apr 30 '17 at 9:08
• thank you so much for this great explanation, I have a long way to go I've just started coding and your advises were surely helpful! – Salah Eddine Apr 30 '17 at 10:10
• @holroy However, people who would Google, find this and copypaste the code from the accepted answer would still have to do something with the code (most likely), which is positive. – htmlcoderexe Apr 30 '17 at 18:49
• 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()):


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."""
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

"""Download jpg images from html at given url to the local filesystem.