Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

This is a Python script to save imgur pictures posted to 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 not images from imgur pages and lacks the ability to get image albums.

# purpose:   downloads images from links posted to specified reddits
# platform:  python 3.2
# references:
#  - reddit JSON --

import json
import urllib.request
import os, sys

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

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

def makeFileExt(content_type):
    "Return extension for specified content type"
    return {
        }.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):
    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:
            # 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:
        except Exception:
            print("ERROR: bad request --", title)

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

def main():
    "Use app as console program."
    if len(sys.argv) > 1:
        # specify your default reddits here

if __name__ == '__main__':
share|improve this question
  • 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(, 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 that you could consider using.

share|improve this answer

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.
share|improve this answer

Your Answer


By posting your answer, you agree to the privacy policy and terms of service.

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