I'm very new to Python and would like some feedback on my script. I'm fairly clueless to best practices, code correctness etc. so if there's anything at all that looks wrong, isn't 'pythonic' or could be done in a simpler way, I'd love if you could point me in the right direction.

#!/usr/bin/env python

"""Follows Instagram users with similar taste and likes their photos.

Scrapes users who have liked a seed user's recent photos (avoiding
users whose profiles seem spammy), then likes some of their most
popular recent photos and follows them. After 3 days, unfollows them.

Required modules:

Version: 2.1.8

Licensed under a BSD New license.

Uses the https://github.com/Instagram/python-instagram client.

import json
import logging
import os
import random
import re
import time
from instagram import client

    'client_id': '',
    'client_secret': '',
    'redirect_uri': '',
    'access_token': '',
    'client_ips': ''
SEED_USER = 'kevin'

# Logging stuff
logging.basicConfig(level=logging.DEBUG, format='%(asctime)s - %(name)s - %(levelname)s - %(message)s')
logger = logging.getLogger(__name__)

# Global declarations
TILES_PATH = os.getcwd()+'/Tiles.json'

def username_to_id(username):
    """Accepts a username and returns its ID."""
    user = api.user_search(q=username, count=1)
    if username != user[0].username:
        logger.error('Username to ID failed')
    return user[0].id

def check_user(user, ids_to_avoid=[]):  # TODO: Check user not super popular
    """Checks if user meets criteria to be followed, returns boolean.

    user (object): An instagram.models.User object
    ids_to_avoid (list): IDs to avoid, defaults to empty list
    if (
        user.profile_picture != 'http://images.ak.instagram.com/profiles/anonymousUser.jpg'
        and user.full_name
        and user.bio
        and re.search(r'follow|f4f|1D|one ?direction|bieber|shout', user.bio, re.I) is None
        and user.id not in ids_to_avoid
        rel = api.user_relationship(user_id=user.id)
        if (
            rel.outgoing_status == 'none'
            and rel.incoming_status != 'followed_by'
            and rel.target_user_is_private is False
            return True
        return False

    while True:

        api = client.InstagramAPI(**CONFIG)

        # Load Tiles.json
        tiles = {}
        with open(TILES_PATH) as f:
            tiles = json.load(f)

        # Make a list of users who are currently being followed, or have been followed before
        already_followed = []
        for tile in tiles['present']:
        for tile in tiles['past']:

        # Scrape users
        scraped_users = []

        def scrape_users():
            next_url = ''
            while len(scraped_users) < NUM_TO_FOLLOW:

                recent_media, next_url = api.user_recent_media(user_id=username_to_id(SEED_USER), count=2, with_next_url=next_url)
                for media in recent_media:

                    likes = api.media_likes(media_id=media.id)
                    for user in likes:

                        if check_user(user=user, ids_to_avoid=(already_followed + scraped_users)):

                            logger.info('Scraped user ' + user.id)

                            if len(scraped_users) >= NUM_TO_FOLLOW:
                            logger.info('Avoided user ' + user.id)

        logger.info('Following and liking the photos of %s users', len(scraped_users))

        # Loop through scraped_users and like their photos and follow them
        for user_id in scraped_users:
                recent_media, next_url = api.user_recent_media(user_id=user_id, count=12)

                media_dict = {}
                for media in recent_media:
                    media_dict[media.like_count] = media.id

                i = 1
                for likes in sorted(media_dict.keys(), reverse=True):

                    if not 0 < likes < 300:

                    if (random.random() + (i / (1 / 0.07))) < 0.5 or i <= 2:

                        api.like_media(media_id=media_dict[likes])  # like_media doesn't return anything?
                        logger.info('Liked media ' + media_dict[likes])
                        time.sleep(random.randint(20, 50))

                    i += 1

                follow = api.follow_user(user_id=user_id)
                if follow[0].outgoing_status != 'none':

                    tiles['present'].append({'user_id': user_id, 'time_followed': time.time()})
                    logger.info('Followed user ' + user_id)

            except Exception, e:

        # Work out who (if anyone) is due for unfollowing
        to_unfollow = []
        for tile in tiles['present']:
            if (time.time() - tile['time_followed']) > (60 * 60 * 24 * 3):
                    if len(to_unfollow) >= NUM_TO_UNFOLLOW:

        logger.info('Unfollowing %s users', len(to_unfollow))

        # Unfollow users due for unfollowing
        for tile in to_unfollow:
                unfollow = api.unfollow_user(user_id=tile['user_id'])
                if unfollow[0].outgoing_status == 'none':

                    logger.info('Unfollowed user ' + tile['user_id'])

            except Exception, e:

        with open(TILES_PATH, 'w') as f:
            json.dump(tiles, f)

        logger.info('Waiting 1 hour until repeat')
        time.sleep(60 * 60)

except KeyboardInterrupt:
    # ^C exits the script: Save Tiles.json first
    with open(TILES_PATH, 'w') as f:
        json.dump(tiles, f)
    logger.info('Saved and exited')

1 Answer 1


You have an error in check_user:

def check_user(...):
    if (...):
        rel = api.user_relationship(...)
        if (...):
            return True
        return False

If the first check passes but the second doesn't, this will implicitly return None. The minimal fix is:

def check_user(...):
    if (...):
        rel = api.user_relationship(...)
        if (...):
            return True
    return False

You have a lot of code there without too much structure. First off, it's general practice to have a defined entry point function (usually main) that is called as follows, at the end of the script:

if __name__ == '__main__':

This means "if the script is being run directly (i.e. not imported), call main".

It is not clear why scrape_users is defined inline like that. I would move it up with the other function definitions, and make e.g. api a parameter. In general, you could use more explicit parameters and return values, rather than relying on scoping.

You could also split out some of the other functionality into separate functions, e.g. unfollow and follow_and_like. This would make your main very simple and avoid the current situation where you can't see the try and except KeyboardInterrupt without a lot of scrolling.

Good work on following the style guide. One minor thing (emphasis mine):

Imports should be grouped in the following order:

  • standard library imports
  • related third party imports
  • local application/library specific imports

You should put a blank line between each group of imports.

Finally, you could consider giving this a CLI to provide the various config values. Look into e.g. argparse for dealing with arguments.

  • \$\begingroup\$ Thanks for the response Jon. Is it good practice to have your functions not rely upon scoping? For example like you mention, using api as a parameter in all of them. Would I put my constant declarations and logging code in main() too? And what about the imports? \$\endgroup\$
    – anon
    Aug 2, 2014 at 12:14
  • \$\begingroup\$ The whole point of import is to bring other names into scope, so you don't need to pass those explicitly (and they should always be at the top of the script). Generally, prefer explicit parameters to global variables: it makes it easier to see what the function is using (which is also why from foo import * is not recommended) and makes testing easier. I'm not sure what the best practice is for logging. \$\endgroup\$
    – jonrsharpe
    Aug 2, 2014 at 12:24