Tweeting NASA's Astronomy Pic of the Day I want to know how I can make this old code faster, cleaner and compact. It may look like bad code because I did it when I first started with Python.

# -*- coding: utf-8 -*-

import urllib
import time
import os
from twython import Twython
from bs4 import BeautifulSoup as bs

class Alfred():

    def __init__(self):
        self.url = "http://apod.nasa.gov/apod/astropix.html"
        self.time = time.localtime()
        self.app_key = "XXX"
        self.app_sec = "XXX"
        self.token_key = "XXX"
        self.token_sec = "XXX"
        self.img_path = "static/img/"

    def start(self):

        tw = Twython(self.app_key, self.app_sec, self.token_key, self.token_sec)

        while True: 

            if self.time[3] &lt; 5:
                web = urllib.urlopen(self.url)
                soup = bs(web, "html.parser")
                img = soup.find("img")
                img_src = img.get("src")
                img_name = img_src[11:]

                if os.path.isfile(self.img_path + img_name):

                    pass

                else:

                    img_list = os.listdir(self.img_path)

                    for file_name in img_list:
                        os.remove(self.img_path + file_name)

                    img_url = "http://apod.nasa.gov/apod/" + img_src
                    urllib.urlretrieve(img_url, 'static/img/' + img_name)
                    photo = open(self.img_path + img_name, 'rb')
                    response = tw.upload_media(media=photo)
                    tw.update_status(status="Imagen astronómica del día. (vía: NASA). ~Alfred", media_ids=[response['media_id']])

            else:

                pass



a = Alfred()
a.start()
Instance and class variables.

I think tw should be an instance variable, made in the constructor.  Same for web and soup - not having to remake those every time will probably help you with your runtime issues (at least a little).

Then, I'd make all of your constants class-level variables.  This includes self.url, self.app_key, self.app_sec, self.token_key, self.token_sec, and self.img_path.  Really, the only one of the instance variables you set there that wouldn't be constant is the time.'

After that, you should probably add a few constants - "http://apod.nasa.gov/apod/" and "Imagen astronómica del día. (vía: NASA). ~Alfred" come to mind.

Then I'd go one step further and put all of the constants into a config file.  If you aren't interested in doing that, skip the rest of this section.

I assume that these may eventually change, or you might want to share/open-source your code and you probably don't want this out there publicly with all those values. Luckily, Python provides ConfigParser to handle config files.  I'd suggest the following structure for your config file



[TwitterBot]
url: http://apod.nasa.gov/apod/astropix.html
app_key: XXX
app_sec: XXX
token_key: XXX
token_sec: XXX
img_path: static/img/


Pretty standard, Windows INI file style config file.  This should either be in the same directory the file is in, or in the user's home directory.  Then you can get the parser object pretty easily - I encapsulated the behavior in a class, you can do whatever works for you.

from ConfigParser import SafeConfigParser

class Configuration(object):
    _parser = None
    _url = None
    _app_key = None
    _app_sec = None
    _token_key = None
    _token_sec = None
    _img_path = None

    def __init__(self, filepath=None):
        self.filepath = filepath

    @property
    def parser(self):
        if self._parser is None:
            self._parser = self._get_config_file(self.filepath)
        return self._parser

    @property
    def url(self):
        return self._get_config_value("url")

    @property
    def app_key(self):
        return self._get_config_value("app_key")

    @property
    def token_key(self):
        return self._get_config_value("token_key")

    @property
    def token_sec(self):
        return self._get_config_value("token_sec")

    @property
    def img_path(self):
        return self._get_config_value("img_path")

    def _get_config_value(self, value_name):
        """Gets a configuration value

        Parameters
        ==========
        value_name : str
            The name of the value you want.

        Returns
        =======
        value : str
            The value you want.
        """

        value = getattr(self, "_{}".format(value_name))
        if value is None:
            return self.parser.get("TwitterBot", value_name)
        return value

    def _get_config_file(self, filepath=None):
        """Gets the config file for the twitter bot

        Parameters
        ==========
        filepath: string, optional
            The path to the desired file.  If omitted, the function first
            looks in the local directory for a file named ".twbot.config", 
            then looks in the user's home directory.

        Returns
        =======
        parser: SafeConfigParser
            A parser object with the config file loaded.
        """

        if filepath is None:
            true_filepath = ".twbot.config"
            if not os.path.isfile(true_filepath):
                true_filepath = os.path.expanduser("~/.twbot.config")
                if not os.path.isfile(true_filepath):
                    raise OSError("Couldn't find '.twbot.config' either "
                                  "locally or in the user directory")
        else:
            true_filepath = filepath

        parser = SafeConfigParser()
        with open(true_filepath, 'r') as config_file:
            parser.readfp(config_file)

        return parser


Use new style classes

Python allows you to use new-style classes (in 2.7) by inheriting from object - this is almost always something you want to be doing.

class Alfred(object):


Naming

I don't like the name start - it isn't very descriptive to me.  What are you starting?  It might be better with a more descriptive class name, but I think you can change it to something like startTweetingImages and be fine.

Break up your start function

Right now there are three distinct functions happening here - getting the image, deleting old files, and then tweeting the image.  I'd make a function for each of them.

def _get_image(self):
    img = self.soup.find("img")
    img_src = img.get("src")
    return img_src, img_src[11:]

def _remove_old_files(self):
    for file_name in os.listdir(self.img_path):
        os.remove(os.path.join(self.img_path, file_name))

def _send_tweet(self, img_src, img_name):
    img_url = self.base_nasa_url + img_src
    image_path = os.path.join(self.img_path, img_name)
    urllib.urlretrieve(img_url, image_path)
    with open(image_path, 'rb') as photo:
        response = self.tw.upload_media(media=photo)
        self.tw.update_status(
            status=self.status, media_ids=[response['media_id']])

def start(self):
    while True: 
        if self.time[3] &lt; 5:
            img_src, img_name = self._get_image()

            if not (os.path.isfile(os.path.join(self.img_path, img_name))):
                self._remove_old_files()
                self._send_tweet(img_src, img_name)


You'll notice I did a few other things - I removed the unnecessary else blocks, and then negated the condition of the if block so we could remove that else as well.  I also used a context manager, safely closing the file when I'm done with it.  Lastly, I used os.path.join to safely join together parts of a filepath so that it is operating system independent.  

The above implementation assumes that you've made things constants that I mentioned should be constants, and that tw, web, and soup are each instance variables. 

Sorry, I got lazy adding docstrings but you should always have them for functions. Sometimes you can omit them for private functions (prepended with a _) but I like them for the convenience of the implementor.  I've used the numpydoc style here.

Lastly, please put things that run (i.e. a = Albert(); a.start()) inside of an

if __name__ == '__main__':


block - this lets people import your module safely and do other stuff with it.