3
\$\begingroup\$

I wrote a simple image scraper for the Desktop Wallpaper project by The Fox is Black. The site is a fantastic (yet, apparently dead) blog full of fabulous images. However, there's no way to download an entire bundle of images, hence this simple scraper.

I'm putting the code up for scrutiny. Any areas for code style and/or implementation improvements? I'd appreciate your feedback.

#!/usr/bin/python

import os
import requests


from bs4 import BeautifulSoup as bs


MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = 'fox_backgrounds'
BASE_URL = "http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page"
RESOLUTIONS = [
            "1280x800", "1440x900", "1680x1050", "1920x1200", "2560x1440",
            "iphone", "iphone-5", "iphone6", "iphone-6-plus", "iphone6plus",
            "ipad",
            ]


def fetch_url(url):
    return requests.get(url).text


def clip_url(href):
    return href[href.rfind('/'):]    


def save_image(href):
    print(f"Downloading: {clip_url(href)}")
    request = requests.get(href)
    with open(f"fox_backgrounds{clip_url(href)}", 'wb') as output:
        output.write(request.content)


def get_images_from_page(url):
    html = fetch_url(url)
    soup = bs(html, "html.parser")
    for link in soup.find_all("a", class_="btn_download"):
        href = link["href"]
        for resolution in RESOLUTIONS:
            if resolution in href:
                save_image(href)


def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)


def get_backgrounds():
    make_dir()
    for page in range(0, MAX_PAGES):
        get_images_from_page(BASE_URL + str(page + 1))


def main():
    get_backgrounds()


if __name__ == '__main__':
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ This was fun to work on. There was a moment of trepidation when I didn't know what kind of "pictures" this site hosted, but it worked out in the end. \$\endgroup\$ – Reinderien Jul 10 at 15:00
  • \$\begingroup\$ I'm glad you enjoyed it! \$\endgroup\$ – baduker Jul 10 at 15:13
2
\$\begingroup\$

Suggested:

#!/usr/bin/python

import os
import requests


from bs4 import BeautifulSoup as bs
from pathlib import Path
from shutil import copyfileobj


MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = Path('fox_backgrounds')
BASE_URL = 'http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page'
RESOLUTIONS = {
    '1280x800', '1440x900', '1680x1050', '1920x1200', '2560x1440',
    'iphone', 'iphone-5', 'iphone6', 'iphone-6-plus', 'iphone6plus',
    'ipad'
}


def fetch_url(url):
    return requests.get(url).text


def clip_part(href):
    return href.rpartition('/')[-1]


def save_image(href):
    part = clip_part(href)
    print(f'   Downloading: {part}')
    fn = SAVE_DIRECTORY / part
    with requests.get(href, stream=True) as response, \
         open(fn, 'wb') as output:
        copyfileobj(response.raw, output)


def get_images_from_page(url):
    html = fetch_url(url)
    soup = bs(html, 'html.parser')
    for link in soup.find_all('a', class_='btn_download'):
        href = link['href']
        if any(href.endswith(f'-{res}.jpg') for res in RESOLUTIONS):
            save_image(href)
        else:
            print(f'Unknown resolution {href}')


def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)


def get_backgrounds():
    make_dir()
    for page in range(1, MAX_PAGES+1):
        print(f'Fetching page {page}...')
        get_images_from_page(f'{BASE_URL}{page}')


def main():
    get_backgrounds()


if __name__ == '__main__':
    main()

Comments:

  • You initialized SAVE_DIRECTORY but then use it for creation and not file write
  • RESOLUTIONS should be a set, or maybe a tuple
  • clip_url is slightly misleading; it returns a URL part and not a whole URL
  • It's best if you have a prompt before downloading the index page; otherwise it hangs without the user knowing what's happening
  • Stream your content so that you don't use up memory for big files
  • Your RESOLUTIONS check is a little puzzling. Maybe it's a validation step? But if it's a validation step, you silently fail instead of printing a warning. Also, you keep iterating even after you've found the correct resolution. I rewrote this to just check the current resolution, and also be a little bit more careful about where it's seen in the filename.
  • range(0, ...) is redundant, but for your use case you're better off with range(1 anyway.
  • rpartition does basically the same thing as what you wrote, but doesn't require any fancy array slicing
  • Don't call clip_path twice

Edit

The following version makes sane use of generators so that the iteration functions only need to know about their iteration, and not the inner business logic.

Also, your resolution check needs to be case-insensitive for many of the files on the site; and the site has gif and png images as well as jpg. You were missing some resolutions and some alternate iPhone spellings. I don't think that it's worth doing a resolution check at all, especially given these edge cases, but I left it in.

#!/usr/bin/python

import os
import requests


from bs4 import BeautifulSoup as bs
from pathlib import Path
from shutil import copyfileobj


MAX_PAGES = 1  # Max. number of pages is 41
SAVE_DIRECTORY = Path('fox_backgrounds')
BASE_URL = 'http://www.thefoxisblack.com/category/the-desktop-wallpaper-project/page'
RESOLUTIONS = {
    '1280x800', '1440x900', '1680x1050', '1920x1200', '2560x1440', '3840x2400',
    'iphone', 'iphone5', 'iphone-5', 'iphone6', 'iphone-6-plus', 'iphone6plus', 'iphone6-plus',
    'ipad'
}


def clip_part(href):
    return href.rpartition('/')[-1]


def save_image(href):
    part = clip_part(href)
    print(f'   Downloading: {part}')
    fn = SAVE_DIRECTORY / part
    with requests.get(href, stream=True) as response, \
         open(fn, 'wb') as output:
        copyfileobj(response.raw, output)


def urls_from_page(url):
    soup = bs(requests.get(url).text, 'html.parser')
    for link in soup.find_all('a', class_='btn_download'):
        href = link['href']
        if any(href.lower().contains(f'-{res}.') for res in RESOLUTIONS):
            yield href
        else:
            print(f'Unknown resolution {href}')


def make_dir():
    os.makedirs(SAVE_DIRECTORY, exist_ok=True)


def all_urls():
    for page in range(1, MAX_PAGES+1):
        print(f'Fetching page {page}...')
        yield from urls_from_page(f'{BASE_URL}{page}')


def main():
    make_dir()
    for url in all_urls():
        save_image(url)


if __name__ == '__main__':
    main()
\$\endgroup\$
  • 1
    \$\begingroup\$ This a great review. Thank you! Even though, this is a relatively short and simple script, I've still missed a couple of things here and there, but that's a good learning experience for me. I appreciate your time and effort! \$\endgroup\$ – baduker Jul 10 at 15:12

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.