5
\$\begingroup\$

I have written a Reddit bot in Python (code below). Open to any improvements, especially on efficiency, as I am using PythonAnywhere to run it.

The main function is to scan posts in reddit and when certain phrases are found in the text private message the reddit user. Furthermore, the reddit user shouldn't be messaged twice.

#------------------------------------------------------------------------------   
def Github_text():

    print("Open textfile...")

    data = urllib.request.urlopen("####").read()
    text = re.search("<StartText>(.+?)<EndText>", str(data))

    if text:
        private_message_text = text.group(1)
        private_message_text = private_message_text.replace(r"\\n","\n")

    print("Text is ready!")

    return private_message_text

def bot_login():

    print("Login...")

    reddit = praw.Reddit(client_id="####",
                client_secret="####",
                password="####",
                user_agent="####",
                username="####")

    print("Login succesfull!")

    return reddit

def run_bot(reddit):

    now = datetime.datetime.now()
    print("Start iteration..." + now.strftime("%Y-%m-%d %H:%M:%S"))

    for submission in reddit.subreddit("all").new(limit=1000):
        if any(sentence in submission.selftext for sentence in check_sentences) \
        and submission.author not in users_messaged and not submission.stickied and submission.is_self: 

          print(submission.selftext)

          if len(users_messaged) > 50: 
              del users_messaged[0] 
              users_messaged.append(submission.author) 
          else:
              users_messaged.append(submission.author)

          reddit.redditor(str(submission.author)).message(private_message_title, private_message_text)

          print("Message send to:" + str(submission.author))

    time.sleep(120)

#------------------------------------------------------------------------------
check_sentences = ["####",
                   "####",
                   "####",
                   "####",
                   "####",
                   "####",
                   "####",
                   "####"]

users_messaged = [] 
private_message_title = "####" 
private_message_text = Github_text()
reddit = bot_login()

print("Start loop!")

while True:
    run_bot(reddit)
\$\endgroup\$
  • 6
    \$\begingroup\$ Hi! This code present issues with its indentation and can not work in its current shape. Can you edit your question to reflect the actual indentation of your code, thank. \$\endgroup\$ – 409_Conflict Sep 3 '18 at 10:15
  • \$\begingroup\$ My bad! I am in class will do in a few hours, thanks for pointing out. \$\endgroup\$ – Duncan Jansen Sep 3 '18 at 11:47
  • 1
    \$\begingroup\$ @DuncanJansen: I removed one layer of indentation on the first two functions. If that is not how your codea ctually looks, please edit it later. \$\endgroup\$ – Graipher Sep 3 '18 at 12:00
  • 1
    \$\begingroup\$ As I am looking at it now, the indentation must be correct. \$\endgroup\$ – Duncan Jansen Sep 3 '18 at 14:15
5
\$\begingroup\$

Pass values as parameters for modularity

You can make your code a lot more modular if you pass values to functions as parameters instead of hardcoding values in the function itself.

Let's change the bot_login() function to this.

def bot_login(client):
    print("Login...")
    reddit = praw.Reddit(client_id=client["id"],
                client_secret=client["secret"],
                password=client["password"],
                user_agent=client["useragent"],
                username=client["username"])
    print("Login succesfull!")
    return reddit

This way you can call the function like this

client={ "id":"1",
    "secret":"supersecret",
    "password":"password123",
    "useragent":"Chrome 1.0",
    "username":"beepbot"}
bot_login(client)

Your Github_text() function should look something like this.

def github_text(github_url):

Also, notice the lowercase function definitions. You are better off following standards. Similarly, you should never go with generic variable names such as data text even though they are better than x,y,t1 etc. You can use them if the context is very clear but even so using specific names such as website_response is better.

You can change the function definition of run_bot(reddit) to run_bot(reddit,subreddit,limit) This way you can maybe run your bot in other subreddits.

Code is better understood vertically

if any(sentence in submission.selftext for sentence in check_sentences) \
        and submission.author not in users_messaged and not submission.stickied and submission.is_self: 

That is very hard to read.

Maybe write it like this

conditions=[any(sentence in submission.selftext for sentence in check_sentences),
            submission.author not in users_messaged,
            not submission.stickied,
            submission.is_self]
if all(conditions):

Please handle edge cases

What happens to the github_text() function if re.search() does not return anything? private_message_text is not defined! Probably won't happen but nothing is guaranteed.

Also if you are using Python3 why not use urllib2 instead of urllib. urlopen() seems to be deprecated use urllib2.urlopen() instead.

Use if __name__=='__main__'

It is better to use this format to run code.

if __name__=='__main__':
    main()

It is a good programming habit even if you are not going to import this anywhere.

  if len(users_messaged) > 50: 
      del users_messaged[0] 
      users_messaged.append(submission.author) 
  else:
      users_messaged.append(submission.author)

I don't know what you are trying to do with this piece of code. But it probably does not do what you want. Since you don't want to message the same person again.

Perfomance

The major performance bottleneck you will face is probably in the network and not in your code. (I could be wrong). I am not familiar with the Reddit API. But if it allows you to message a bunch of users at the same time then you should probably generate the user list and message them all at once.

As a side note, you should handle exceptions if there are any. Especially, if you are going to run this code for a long time. You don't want your program to suddenly stop because of a read timeout.

\$\endgroup\$
  • \$\begingroup\$ Hey, thanks! I will be reviewing and implementing what you have told me this in the next days! \$\endgroup\$ – Duncan Jansen Sep 3 '18 at 16:04
  • 2
    \$\begingroup\$ I think the point of deleting the first user if the number of messaged users is larger than 50 is to keep only the last 50 messaged users in memory. If that is the case, I would recommend using a collections.deque, which has an optional maximum length argument. \$\endgroup\$ – Graipher Sep 3 '18 at 16:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

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