Reddit-Bot sucht & ersetzt Suchbegriffe - Feedback erwünscht

[Gerion]

Hi Leute,

ich habe mir einen simplen Reddit-Bot programmiert, der in einem Subreddit alle Beiträge und Kommentare durchsucht, Suchbegriffe ersetzt und den korrigierten Text als Antwort postet.

In Zukunft möchte ich den Bot noch weiter ausbauen, bspw. sodass er permanent auf einem Server läuft anstatt immer lokal gestartet zu werden. Vorher gibt es aber sicherlich an meinem bestehenden Code noch Optimierungspotenzial, daher würde ich mich über Verbesserungsvorschläge freuen.

Hier noch der Link zum zugehörigen Github-Projekt: https://github.com/Kaefertal/reddit-com ... d-replacer

Code: Alles auswählen

import time
import praw
from random import randint
import ast

#Import username and password.
with open("username_password") as f:
    USERNAME = f.readline()[:-1]
    PASSWORD = f.readline()
f.closed

#Declare WAIT and MAXPOSTS for Reddit.
WAIT = 20
MAXPOSTS = 1000

#Declare subreddit.
sub = "YOUR DESIRED SUBREDDIT"

#Import array of expressions to find and replace.
with open("to_find") as f:
    to_find = ast.literal_eval(f.read())
f.closed

#Define signature.
signatur = "YOUR SIGNATURE"

#Set user agent and login to Reddit.
user_agent = ("YOUR USER AGENT")
r = praw.Reddit(user_agent=user_agent)
r.login(USERNAME, PASSWORD)


#Shorten reply if there is too much space between the replacements.
def shorten_reply(current_element, positions):
    
    positions.sort()
    i = 0
    left_out = 0
        
    while i < len(positions) - 1:
        #Check space between replacements.
        if positions[i+1][0] - positions[i][1] > 120:
            
            #Define start of the cut: Find " " in current_element from end of current replacement + buffer - left_out.
            cut_start = current_element[:positions[i][1] + 50 - left_out].rfind(" ")
            #Define end of the cut: Find " " in current_element from start of next replacement - buffer - left_out. This is 0, so we add end of next replacement - buffer - left_out. 
            cut_end = current_element[positions[i+1][0] - 50 - left_out:].find(" ") + positions[i+1][0] - 49 - left_out
            
            #Cut space out.
            current_element = current_element[:cut_start] + " [...] " + current_element[cut_end:]
            
            #Calculate how much was cut out.
            left_out = left_out + cut_end - cut_start - 7
        
        i += 1
            
    else:
        #Check space from last replacement to end of element.
        if positions[i][1] - left_out - len(current_element) < -120:
            cut_start = positions[i][1] + 50 - left_out
            current_element = current_element[:cut_start] + " [...] "  
    
    return current_element
        

#Check if the author of the element is blacklisted or the element has already been replied to.
def author_check_elements(element, kind):
    author_blacklist = ["Author_1", "Author_2"]
    try:
        print("Checking author of element " + element.id)
        if str(element.author) in author_blacklist:
            print("Skipping element, author is " + str(element.author))
            return True
    except AttributeError:
        element.author = "[DELETED]"
        
    if kind == "posts":
        answers = element.comments
    elif kind == "comments":
        answers = element.replies
    else:
        print("Something went wrong with the answers to the element.")
        
    try:
        for c in answers:
            print("Checking authors of replies to element " + c.id)
            if str(c.author) == USERNAME:
                print("Skipping element, " + str(c.author + " posted in comments"))
                return True
    except(AttributeError):
        c.author = '[DELETED]'


#Main function. Searches for the defined keywords in the fetched comments and replaces them.
def search_comments(elements, kind):
    for element in elements:
        print("Next element: " + element.id)
        if author_check_elements(element, kind):
            continue
        else:
            print("Author of the element and its direct replies are OK. Going on.")
        
        positions = []
        
        #Declare the item which is to be examined as the comment-body or the post's selftext respectively.
        if kind == "posts":
            current_element = element.selftext
        elif kind == "comments":
            current_element = element.body
        else:
            print("Something is wrong with the analyzed element.")
            
        #print("Looking for words.")
        i = 0
        replaced = False
        while i < len(to_find):
            
            key = to_find[i][0]
            
            #Check if there are more then two replacements. If so, choose one randomly.
            if len(to_find[i]) > 2:
                new = to_find[i][randint(1, len(to_find[i]) - 1)]
            else:
                new = to_find[i][1]
            #Search for keywords, replace them, save the position of each replacement.
            while key in current_element.lower():
                pos = current_element.lower().find(key)
                current_element = current_element[:pos] + new + current_element[pos+len(key):]
                replaced = True
                print("Replacing " + key + " with " + new + ".")
                positions.append([pos, pos+len(new)])
            i += 1
        
        #Try to shorten the comment if keywords were found and replaced.
        if replaced == True:
            new_comment = shorten_reply(current_element, positions)
            #Post the edited element as a reply to the original element.
            element.reply("YOUR INTRO " + new_comment + signatur)
        else:
            print("Nothing found in element " + element.id)
        

#Execute everything. Fetch subreddit, extract posts and comments, execute main functions, catch exception, after finishing wait and repeat.
while True:
    print("Fetching subreddit.")
    subreddit = r.get_subreddit(sub)
    subreddit_posts = subreddit.get_new(limit=MAXPOSTS)
    subreddit_comments = subreddit.get_comments(limit=MAXPOSTS)
    
    try:
        kind = "posts"
        print("Checking posts.")
        search_comments(subreddit_posts, kind)
    except Exception as e:
        print("An error has occured while checking posts: " + str(e))

    try:
        kind = "comments"
        print("Checking comments.")
        search_comments(subreddit_comments, kind)
    except Exception as e:
        print("An error has occured while checking comments: " + str(e))

    print("Finished. Starting again in " + str(WAIT) + " seconds.")
    
    time.sleep(WAIT)


#Print final message before the bot stops. 
print("For some reason the robot stopped.")

[BlackJack]

@Gerion: Da ist einiges sehr merkwürdig dran, sowohl allgemein, als auch speziell auf Python bezogen. Bevor man über Optimierungen nachdenkt sollte man da einiges erst einmal sauberer schreiben und vor allem bin ich mir nicht sicher ob das überhaupt korrekt ist was da gemacht wird. Es werden sich ja beispielsweise die Positionen der Ersetzungen gemerkt. Das kann aber soweit ich das sehe nur funktionieren wenn die Ersetzungen immer genau so lang sind wie die Suchworte, denn ansonsten verschieben sich ja die Positionen die man sich von vorherigen Ersetzungen gemerkt hat. Falls nur gleiche Längen erlaubt sind, sollte man das im Code sicherstellen.

Auf Modulebene steht direkt ein Teil des Hauptprogramms, und das auch noch mit Funktionsdefinitionen vermischt. Das ist sehr unübersichtlich und man kann das Modul nicht ohne Nebeneffekte importieren. Auf Modulebene sollten nur Konstanten, Funktionen, und Klassen definiert werden. Das Hauptprogramm gehört in eine Funktion und Werte, ausser Konstanten, die eine Funktion verwendet sollten als Argumente die Funktion betreten und nicht einfach so magisch aus der Umgebung kommen. Die Hauptfunktion heisst üblicherweise `main()` und wird mit folgendem Idiom aufgerufen:

Code: Alles auswählen

if __name__ == '__main__':
    main()

Einige Namen könnten deutlich besser sein. Insbesondere die einbuchstabigen, aber auch Abkürzungen sollte man vermeiden. `sub` ist beispielsweise nicht gut, weil man entweder den erklärenden Kommentar suchen oder raten muss, dass dieser Name eigentlich `subreddit_name` heissen sollte.

Die Kommentare vor den Funktionen würden sich als DocStrings für die Funktionen anbieten. Einige Kommentare sind überflüssig weil sie das offensichtliche was dort als Code steht einfach noch einmal wiederholen.

``f.closed`` als einzelne Anweisung hat keinen Effekt. Was hast Du Dir dabei gedacht?

Anstelle von `ast.literal_eval()` zu verwenden sollte man ein Datenformat wählen das nicht Python-spezifisch ist. Zum Beispiel CSV oder JSON. Für beides bietet die Standardbibliothek Module. Die Datenstruktur von dieser 'to_find'-Datei ist auch nicht schön. Da sind Sequenzen enthalten bei denen Position 0 eine andere Bedeutung hat als die restlichen Positionen, die alle die gleiche Bedeutung haben. Es sind also Elemente mit verschiedenen Bedeutungen auf einer Ebene in der Struktur, was zu so einem gruseligen Code wie ``to_find[randint(1, len(to_find) - 1)]`` anstatt das man einfach `random.choice()` verwenden könnte. Das ``if`` in dem dieser Schnippsel verwendet wird ist überflüssig, denn wenn man das und den ``else``-Zweig einfach weglässt dann hat man exakt den selben Effekt mit weniger Code.

Neben der Anmerkung mit den Positionen die sich verschieben wenn die Ersetzung nicht die gleiche Länge hat, kann es auch noch passieren das eine spätere Ersetzung Teile von einer bereits erfolgten Ersetzung beinhaltet. Dann kann es überlappende Positionen geben. Kann der Code damit umgehen? Soll das überhaupt passieren dürfen? Eventuell wäre ein komplett anderer Ansatz, zum Beispiel mit dem `re`-Modul den Beitrag zu zerlegen, sinnvoller‽

[Gerion]

Danke für die ausführliche Antwort! Dieser Bot ist bei weitem das "komplexeste" was ich bislang programmiert habe - daher kann ich mir vorstellen, dass noch einiges im Argen liegt.

@Gerion: Da ist einiges sehr merkwürdig dran, sowohl allgemein, als auch speziell auf Python bezogen. Bevor man über Optimierungen nachdenkt sollte man da einiges erst einmal sauberer schreiben und vor allem bin ich mir nicht sicher ob das überhaupt korrekt ist was da gemacht wird. Es werden sich ja beispielsweise die Positionen der Ersetzungen gemerkt. Das kann aber soweit ich das sehe nur funktionieren wenn die Ersetzungen immer genau so lang sind wie die Suchworte, denn ansonsten verschieben sich ja die Positionen die man sich von vorherigen Ersetzungen gemerkt hat. Falls nur gleiche Längen erlaubt sind, sollte man das im Code sicherstellen.

Stimmt eigentlich. Die Positionen müssten sich mit jeder Ersetzung verschieben, aber das geht bestimmt auch einfacher. Die Positionen werden gemerkt, um Kommentare bei großen Abständen zwischen den Ersetzungen zu kürzen. Insofern kommt es bei der Position nicht auf ein oder zwei Stellen an, aber es soll natürlich trotzdem korrekt funktionieren, also genau sein.

Auf Modulebene steht direkt ein Teil des Hauptprogramms, und das auch noch mit Funktionsdefinitionen vermischt. Das ist sehr unübersichtlich und man kann das Modul nicht ohne Nebeneffekte importieren. Auf Modulebene sollten nur Konstanten, Funktionen, und Klassen definiert werden. Das Hauptprogramm gehört in eine Funktion und Werte, ausser Konstanten, die eine Funktion verwendet sollten als Argumente die Funktion betreten und nicht einfach so magisch aus der Umgebung kommen. Die Hauptfunktion heisst üblicherweise `main()` und wird mit folgendem Idiom aufgerufen:

Code: Alles auswählen

if __name__ == '__main__':
    main()

Einige Namen könnten deutlich besser sein. Insbesondere die einbuchstabigen, aber auch Abkürzungen sollte man vermeiden. `sub` ist beispielsweise nicht gut, weil man entweder den erklärenden Kommentar suchen oder raten muss, dass dieser Name eigentlich `subreddit_name` heissen sollte.

Die Kommentare vor den Funktionen würden sich als DocStrings für die Funktionen anbieten. Einige Kommentare sind überflüssig weil sie das offensichtliche was dort als Code steht einfach noch einmal wiederholen.

Ok, dann werde ich das alles mal so umbauen. (Das offensichtliche zu kommentieren habe ich mir angewöhnt weil die Entwickler bei mir auf der Arbeit gerne behaupten, ihr Code sei doch selbsterklärend...)

``f.closed`` als einzelne Anweisung hat keinen Effekt. Was hast Du Dir dabei gedacht?

Ich habe mir ergoogelt wie ich den Inhalt von Dateien importieren kann und es wurde oft geschrieben, dass zu einem "open" unbedingt immer ein "closed" gehört... Wenn das unnötig ist, nehme ich es raus.

Anstelle von `ast.literal_eval()` zu verwenden sollte man ein Datenformat wählen das nicht Python-spezifisch ist. Zum Beispiel CSV oder JSON. Für beides bietet die Standardbibliothek Module. Die Datenstruktur von dieser 'to_find'-Datei ist auch nicht schön. Da sind Sequenzen enthalten bei denen Position 0 eine andere Bedeutung hat als die restlichen Positionen, die alle die gleiche Bedeutung haben. Es sind also Elemente mit verschiedenen Bedeutungen auf einer Ebene in der Struktur, was zu so einem gruseligen Code wie ``to_find[randint(1, len(to_find) - 1)]`` anstatt das man einfach `random.choice()` verwenden könnte. Das ``if`` in dem dieser Schnippsel verwendet wird ist überflüssig, denn wenn man das und den ``else``-Zweig einfach weglässt dann hat man exakt den selben Effekt mit weniger Code.

Das "if" kann man eigentlich weglassen, stimmt...

Position 0 ist in meiner "to_find" immer das Suchwort und die folgenden Positionen die Ersetzungen. Aber würde eine CSV-Datei denn wirklich einen Unterschied machen? Damit kann ich doch im Prinzip auch wieder nur Listen darstellen, während ich mit JSON Dictionaries benutzen und insofern den Zufallsgenerator wie von dir beschrieben vereinfachen kann, oder?

Neben der Anmerkung mit den Positionen die sich verschieben wenn die Ersetzung nicht die gleiche Länge hat, kann es auch noch passieren das eine spätere Ersetzung Teile von einer bereits erfolgten Ersetzung beinhaltet. Dann kann es überlappende Positionen geben. Kann der Code damit umgehen? Soll das überhaupt passieren dürfen? Eventuell wäre ein komplett anderer Ansatz, zum Beispiel mit dem `re`-Modul den Beitrag zu zerlegen, sinnvoller‽

Mit meiner tatsächlich verwendeten Liste passiert das nicht, weil ich beim Aufbau der Liste darauf geachtet habe. Aber der Bot soll ja auch funktionieren wenn ihn jemand anders nutzt. Das "re"-Modul werde ich mir mal angucken.

Wenn ich alle Tipps umgesetzt habe, melde ich mich nochmal mit dem überarbeiteten Code!


Zunächst hätte ich aber nochmal eine ganz andere Frage: Sind solche absoluten Noob-Themen hier eigentlich erwünscht?

[BlackJack]

@Gerion: Dateien die man öffnet sollten auch wieder geschlossen werden, aber dafür muss man die `close()`-Methode aufrufen und nicht das `closed`-Attribut abrufen. Oder man verwendet wie in Deinem Code die ``with``-Anweisung damit die Datei geschlossen wird, dann muss man das nicht noch mal explizit mit einem `close()`-Aufruf tun.

Wenn man eine CSV-Datei verwendet dann hat man wieder nur ”flache” Listen, aber das könnte man beim einlesen der Daten ja in ein Wörterbuch umsetzen das den jeweiligen Wert in der ersten Spalte auf eine Liste mit den restlichen Spaltenwerten abbildet.

Hier mal ein anderer Ansatz zum ersetzen und kürzen (nahezu ungetestet):

Code: Alles auswählen

#!/usr/bin/env python
# coding: utf8
from __future__ import absolute_import, division, print_function
import random
import re


def shorten(string, margin=50, ellipsis=u'[…]'):
    # 
    # Only shorten if replacing something with the ellipsis really *shortens*
    # the string by at least one character.
    # 
    if len(string) > margin * 2 + len(ellipsis) + 2:
        try:
            left_index = string.index(' ', margin)
            right_index = string.rindex(' ', 0, len(string) - margin - 1)
        except ValueError:
            pass  # No space(s) found, so do nothing.
        else:
            if left_index < right_index:
                string = u'{0}{1}{2}'.format(
                    string[:left_index + 1], ellipsis, string[right_index:]
                )
    return string


def replace_and_shorten(text, replacements, shorten_margin=50, ellipsis=u'[…]'):
    regex = re.compile(
        u'(?P<chaff>.*?)(?P<wheat>{0})'.format(
            '|'.join(
                re.escape(string)
                for string in sorted(replacements, key=len, reverse=True)
            )
        )
    )
    result = list()
    match = None
    for match in regex.finditer(text):
        result.append(shorten(match.group('chaff'), shorten_margin, ellipsis))
        result.append(random.choice(replacements[match.group('wheat')]))
    if match:
        result.append(shorten(text[match.end():], shorten_margin, ellipsis))

    return ''.join(result)



def main():
    print(
        replace_and_shorten(
            u'a b c d e f g h i j k',
            {u'b': [u'1', u'2'], 'g': ['x', 'y', 'z']},
            1,
            u'…'
        )
    )


if __name__ == '__main__':
    main()