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] < 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.i

StackExchange Code Review Q#106881, answer score: 2