Download image links posted to reddit.com

Question

This is a Python script to save imgur pictures posted to reddit.com forums. I'm looking for an assessment on the design of this script and any web security issues that might exist.

Obvious shortcomings: it only downloads image links http://i.imgur.com/xx.png not images from imgur pages http://imgur.com/page/ and lacks the ability to get image albums.

# purpose:   downloads images from imgur.com links posted to specified reddits
# platform:  python 3.2
# references:
#  - reddit JSON -- https://github.com/reddit/reddit/wiki/API:-info.json

import json
import urllib.request
import os, sys


def downloadPostsFrom(subreddit):
    "Returns reddit posts as JSON"
    try:
        f = urllib.request.urlopen("http://www.reddit.com/r/"+subreddit+"/.json");
        return json.loads(f.read().decode("utf-8"))["data"]["children"]
    except Exception:
        print("ERROR: malformed JSON response from reddit.com")
        raise ValueError


def parseImageURL_From(posts):
    "Returns tuple iterator (url, title)"
    for node in posts:
        post = node['data']
        # only accept links from imgur.com
        if post['domain'].endswith('imgur.com'):
            yield post["url"], post["title"]


def makeFileExt(content_type):
    "Return extension for specified content type"
    return {
        'image/bmp':'bmp',
        'image/gif':'gif',
        'image/jpeg':'jpg',
        'image/png':'png',
        'image/tiff':'tif',
        'image/x-icon':'ico'
        }.get(content_type, 'txt')


def makeFileName(title):
    "Compute valid file name from image title. Return file name or default.ext"
    title = title.strip(' .?:')
    file_name = title.translate(''.maketrans('\/:*?"<>|$@ .','__.-_____S0___'))
    file_name = file_name.strip('_').replace('__','_').lower()
    return file_name + '.' if file_name else 'default.'


def makeSaveDir(save_dir):
    "Creates directory. Returns a valid directory name."
    if not os.path.exists(save_dir):
        os.makedirs(save_dir)
    return save_dir + '/'


def downloadImagesIntoDir(save_dir, image_refs):
    "Download image files into specified directory."
    save_dir = makeSaveDir(save_dir)
    print('saving to:', save_dir)

    for url, title in image_refs:
        try:
            # open connection to image
            request = urllib.request.urlopen(url)
            if request.code == 200:
                content_type = request.headers.get_content_type()
                # filter in images
                if 'image' == content_type.split('/')[0]:
                    file_name = save_dir \
                            + makeFileName(title) + makeFileExt(content_type)
                    # download image to local file
                    print('  downloading:', title)
                    with open(file_name, "wb") as image_file:
                        image_file.write(request.read())
        except Exception:
            print("ERROR: bad request --", title)


def downloadRedditImages(subreddits):
    "Download imgur images from reddit. Returns None."
    for subreddit in subreddits:
        try:
            # reddit_posts: JSON
            reddit_posts = downloadPostsFrom(subreddit)
            image_urls = parseImageURL_From(reddit_posts)
            downloadImagesIntoDir(subreddit, image_urls)
        except Exception:
            pass


def main():
    "Use app as console program."
    if len(sys.argv) > 1:
        downloadRedditImages(sys.argv[1:]);
    else:
        # specify your default reddits here
        downloadRedditImages(['funny']);


if __name__ == '__main__':
    main()

Answer 1

• Never do except Exception:, especially not to pass. You probably want to be catching KeyErrors specifically, or using dict.get in the first case, and in the other cases adjust accordingly.

• There's no need to do json.loads(file_like_object.read()), just use json.load(file_like_object).

• You can eliminate a race condition in makeSavedir by not calling os.path.exists, just call os.makedirs (and catch the IOError if you'd like to not raise the exception [and make sure to check its errno]).

• You can use string formatting in place of adding strings.

• Your try blocks are too big. They will catch various exceptions that you aren't handling. This is another instance of my first point.

There are other things, these were just the quick things from reading top to bottom :).

P.S. There's an unofficial Python wrapper for the reddit API at https://github.com/praw-dev/praw that you could consider using.

Answer 2

I think, the following line should have a else condtion to print error if it failed.

if request.code == 200:

From the except block, it looks like you want to print errors.

• You can use logging module for logging error and debug statements.
• If the script is intended to be used by others, you can use argparse module for command line arguments.