Web-scraping Reddit Bot

Question

I have been working on a web-scraping Reddit bot in Python 2.7 with the premise of going to /r/eve (a game sub-reddit) finding posts that contain a link to a website hosting killmail information (basically combat logs in-game) and then replying to the post with the TL;DR of the log.

1. Can anyone take a look at my code and help improve it? I've written it entirely independently and I'm still a beginner so I'm sure there's a lot that could be improved.
2. Should I be scraping information differently? (Some variables are a bit hackish.)
3. Is there any pieces of irrelevant or really unoptimized code?
4. Am I doing anything widely considered a bad practice?
5. Can anyone just review my code in general and help me become a better programmer from it?

Here is my complete project (with README and notes).

# EVE: Online Killmail Reddit Bot (EKRB)

import urllib   # Access internet and make network requests
import re       # Regex
import praw     # Python Reddit API Wrapper
from bs4 import BeautifulSoup # Web scraping
import time     # Timer for running the bot every set amount of time
import requests # Allows for catching ConnectionErrors and rerunning the program.

r = praw.Reddit(user_agent='EVE: Online Killmail Reader Bot v1.944 - Created by /u/Valestrum '
                                'Designed to help users get killmail info without clicking links.')
r.login('UsernameHere','PasswordHere')
loopCount = 0

def condense_value(num, suffix='ISK'):
    if num > 999999999999999:
        return("%s %s") % (num,suffix)
    else:
        for unit in ['','thousand','million','billion','trillion']:
            if abs(num) < 1000.0:
                return "%.2f %s %s" % (num, unit, suffix)
            num /= 1000.0

def run_bot():
    with open('cache.txt','r') as cache:
        existing = cache.read().splitlines()

    subreddit = r.get_subreddit("eve")
    comments = subreddit.get_comments(limit=150)

    with open('cache.txt', 'a+') as cache:
        for comment in comments:
            comment_text = comment.body.lower()

            #Records any relevant URLs.
            killmails = [item for item in comment_text.split() if re.match(r"https://zkillboard\.com/kill/*", item)]

            if killmails and comment.id not in existing: #if killmail list is not empty and bot has never messaged
                mails = []
                for mail in killmails:
                    if mail[:13] == 'https://zkill' or mail[:12] == 'http://zkill':
                        mails.append(str(mail))
                existing.append(comment.id)
                cache.write(comment.id + '\n')
                print("I found a new comment! The ID is: " + comment.id)
                report = read_killmail(mails)
                comment.reply(report)

def read_killmail(killmails):
        replyData = []
        for url in killmails:
            soup = BeautifulSoup(urllib.urlopen(url).read())
            iskDropped = soup.find("td", class_="item_dropped").get_text()
            iskDestroyed = soup.find("td", class_="item_destroyed").get_text()
            iskTotal = soup.find("strong", class_="item_dropped").get_text()
            iskDropped, iskDestroyed, iskTotal = [condense_value(int(value[:-7].replace(',',''))) for value in [iskDropped, iskDestroyed, iskTotal]]

            system = soup.find_all('a', href=re.compile('/system/'))[1].get_text() #Ex: Iralaja
            date = soup.find("table", class_="table table-condensed table-striped table-hover").find_all('td')[3].get_text()[:10]
            if len(date) < 6:
                    date = soup.find("table", class_="table table-condensed table-striped table-hover").find_all('td')[2].get_text()[:10]
            otherPilots = int(str(soup.find("th", class_="hidden-md hidden-xs").get_text())[:-9])-1 #Ex: '44' out of "45 Involved", excluded 1 being kb

            #v = victim, kb = pilot firing killing blow
            vPilotInfo = soup.find("table", class_="table table-condensed").find_all('td')[2].get_text().split('\n\n')
            vPilotName = vPilotInfo[0]
            if len(vPilotInfo) > 1:
                    vCorp = vPilotInfo[1]
                    if len(vPilotInfo) > 3: # This accounts for extra variable '' added to PilotInfo
                            vAlliance = vPilotInfo[2]
                    else:
                            vAlliance = '<No Alliance>'
            else:
                    vCorp = '<No Corp>'
                    vAlliance = '<No Alliance>'

            vShipType = (''.join(((soup.find("td", style="width: 100%").get_text())).split())) # Ex: Leviathan(Titan)
            if vShipType[0].lower() in 'aeiou':
                    vShipType = 'n '+str(vShipType)
            else:
                    vShipType = ' '+str(vShipType)
            vRiggingText = soup.find_all('ul', class_="dropdown-menu")[3].find('a').get_text()
            vRiggingLink = soup.find_all('ul', class_="dropdown-menu")[3].find_all('a', href=re.compile('/o.smium.org/loadout/'))[0]['href']

            kbShipType = soup.find_all('tr', class_="attacker")[0].find_all('a', href=re.compile('/ship/'))[0].img.get('alt') #Ex: Nyx
            if kbShipType[0].lower() in 'aeiou':
                    kbShipType = 'n '+str(kbShipType)
            else:
                    kbShipType = ' '+str(kbShipType)
            if int(otherPilots) == 0:
                    kbPilotInfo = soup.find('div', class_="hidden-sm hidden-md hidden-xs").get_text().split('\n\n')
                    kbPilotName = kbPilotInfo[0]
                    if len(kbPilotInfo) > 1:
                            kbCorp = kbPilotInfo[1]
                            if len(kbPilotInfo) > 3:
                                    kbAlliance = kbPilotInfo[2]
                            else:
                                    kbAlliance = '<No Alliance>'
                    else:
                            kbCorp = '<No Corp>'
                            kbAlliance = '<No Alliance>'
                    replyData.append("\n\n>On %s a%s piloted by %s of (%s | %s) was destroyed in system %s by %s of (%s | %s) flying a%s along with %s others." % (date,vShipType,vPilotName,vCorp,vAlliance,system,kbPilotName,kbCorp,kbAlliance,kbShipType,otherPilots))

            else:
                    kbPilotName = soup.find_all('td', style="text-align: center;")[0].find_all('a', href=re.compile('/character/'))[0].img.get('alt')
                    if int(otherPilots) == 1:
                            replyData.append("\n\n>On %s a%s piloted by %s of (%s | %s) was destroyed in system %s by %s flying a%s along with %s other." % (date,vShipType,vPilotName,vCorp,vAlliance,system,kbPilotName,kbShipType,otherPilots))
                    else:
                            replyData.append("\n\n>On %s a%s piloted by %s of (%s | %s) was destroyed in system %s by %s flying a%s along with %s others." % (date,vShipType,vPilotName,vCorp,vAlliance,system,kbPilotName,kbShipType,otherPilots))
            replyData.append("\n\n>Value dropped: %s\n\n>Value destroyed: %s\n\n>Total value: %s\n\n>[%s's %s](%s)\n\n" % (iskDropped,iskDestroyed,iskTotal,vPilotName,vRiggingText,vRiggingLink)+('-'*50))
        replyData = ('\n\n'.join(replyData))

        return("Hi, I am a killmail reader bot. Let me summarize killmail for you!"
        +str(replyData)
        +"\n\n^^This ^^bot ^^is ^^open ^^source ^^& ^^in ^^active ^^development! ^^Please ^^feel ^^free ^^to ^^contribute: ^^[Suggestions](%s) ^^| ^^[Code](%s)") % ('http://www.reddit.com/message/compose?to=Killmail_Bot','https://github.com/ArnoldM904/EK_Reddit_Bot/blob/master/EKR_Bot.py')

while True:
    try:
        run_bot()
    except requests.ConnectionError as e:
        print e
        time.sleep(60)
        run_bot()
    loopCount += 1
    print("Program loop #"+str(loopCount)+" completed successfully.")
    time.sleep(1200)

Answer 1

Some comments based on a brief skim:

• If you want to write Python, you should read PEP 8, the Python style guide. Among other things:

  • Python variables are lowercase_with_underscore, not camelCase.
  • Lines should be wrapped to 79 characters wide.
  • Module imports should be in alphabetical order, and split into groups depending on whether they’re standard library, third-party or project-specific.
  • In a list of values, a space goes after the comma.

  There may be others – this is just what I noticed.

• Your code is very poorly documented. In particular, none of your functions have docstrings, which means I have to read the code to work out what it does. More comments and docstrings will make it easier for somebody else to read/review your code.

• Rather than hard-coding passwords in the file (r.login('username', 'password')), it's better to use something like the keyring module, which can interact with the system keychain. Storing the password in a keychain is more secure than a string in a file.

• I would wrap the if check variable[0].lower() in 'aeiou' in a function as follows:

  def startswith_vowel(string):
      return string.lower()[0] in 'aeiou'
  

  It just makes the code a little cleaner, I think.

• In lines 106–109, when you choose between two strings to append to replyData based on a pluralisation issue, I’d suggest changing this to it’s just the very last part of the string that changes. Right now, if you want to change anything except the last part, you have to change it twice, and this is unnecessary. Consider:

  nextline = "\n\n>On %s a%s piloted by %s of (%s | %s) was destroyed in system %s by %s flying a%s along with %s other" % (date,vShipType,vPilotName,vCorp,vAlliance,system,kbPilotName,kbShipType)
  
  if int(otherPilots) == 1:
      nextline += "s."
  else:
      nextline += "."
  
  replyData.append(nextline)
  

• I haven't been thorough in checking, but there seems to be some casting with str() in places where, at a cursory glance, it isn't necessary.

• In line 41, rather than using an index/slice based comparison, you can used startswith():

  if mail.startswith('https://zkill') or mail.startswith('http://zkill'):
  

  I think that's a little easier to read, and less prone to silly errors in your slice size.

• Rather than running all your main code in the top-level, it’s better to put it in a main() function and then add if __name__ == '__main__': main() at the bottom of the file.

  This allows the file to act as both a script and a module: if you run it directly, then main() is executed, but you can also import functions from it elsewhere, without the main program running upon import.