Trennung von Bedenken: Wann ist es "zu viel" Trennung?

Ich liebe sauberen Code wirklich und ich möchte meinen Code immer auf die bestmögliche Weise codieren. Aber es gab immer eine Sache, die ich nicht wirklich verstand:

Wann ist es zu viel "Trennung von Bedenken" in Bezug auf Methoden?

Angenommen, wir haben die folgende Methode:

def get_last_appearance_of_keyword(file, keyword):
    with open(file, 'r') as file:
        line_number = 0
        for line in file:
            if keyword in line:
                line_number = line
        return line_number

Ich denke, diese Methode ist in Ordnung. Es ist einfach, leicht zu lesen und macht genau das, was der Name sagt. Aber: Es macht nicht wirklich "nur eine Sache". Es öffnet die Datei tatsächlich und findet sie dann. Das heißt, ich könnte es noch weiter aufteilen (auch unter Berücksichtigung des "Single Responsibility Principle"):

Variation B (Nun, das macht irgendwie Sinn. Auf diese Weise können wir den Algorithmus zum Finden des letzten Auftretens eines Schlüsselworts in einem Text leicht wiederverwenden, aber es scheint "zu viel" zu sein. Ich kann nicht erklären, warum, aber ich fühle einfach " " es auf diese Weise):

def get_last_appearance_of_keyword(file, keyword):
    with open(file, 'r') as text_from_file:
        line_number = find_last_appearance_of_keyword(text_from_file, keyword) 
    return line_number

def find_last_appearance_of_keyword(text, keyword):
    line_number = 0
    for line in text:
        if keyword in line:
            line_number = line
    return line_number

Variation C (Dies ist meiner Meinung nach einfach absurd. Wir kapseln einen Einzeiler grundsätzlich mit nur einer Zeile zweimal in eine andere Methode. Man könnte jedoch argumentieren, dass sich die Art und Weise, etwas zu öffnen, aufgrund einiger Funktionsanforderungen in Zukunft ändern könnte und da wir es nicht oft ändern wollen, sondern nur einmal, kapseln wir es einfach und trennen unsere Hauptfunktion noch weiter):

def get_last_appearance_of_keyword(file, keyword):
    text_from_file = get_text_from_file(file)
    line_number = find_keyword_in_text(text_from_file, keyword)
    return line_number 

def get_text_from_file(file):
    with open(file, 'r') as text:
        return text

def find_last_appearance_of_keyword(text, keyword):
    line_number = 0
    for line in text:
        if check_if_keyword_in_string(line, keyword):
            line_number = line         
    return line_number

def check_if_keyword_in_string(text, keyword):
    if keyword in string:
        return true
    return false

Also meine Frage jetzt: Was ist die richtige Art, diesen Code zu schreiben und warum sind die anderen Ansätze richtig oder falsch? Ich habe immer gelernt: Trennung, aber niemals, wenn es einfach zu viel ist. Und wie kann ich in Zukunft sicher sein, dass es "genau richtig" ist und dass es keine weitere Trennung braucht, wenn ich wieder codiere?

Ihre verschiedenen Beispiele für die Aufteilung von Bedenken in separate Funktionen haben alle das gleiche Problem: Sie codieren die Dateiabhängigkeit immer noch hart in get_last_appearance_of_keyword. Dies macht es schwierig, diese Funktion zu testen, da sie jetzt auf eine im Dateisystem vorhandene Datei antworten muss, wenn der Test ausgeführt wird. Dies führt zu Sprödigkeitstests.

Also würde ich einfach Ihre ursprüngliche Funktion ändern in:

def get_last_appearance_of_keyword(text, keyword):
    line_number = 0
    for line in text:
        if keyword in line:
            line_number = line
    return line_number

Jetzt haben Sie eine Funktion, die nur eine Verantwortung hat: Finden Sie das letzte Vorkommen eines Schlüsselworts in einem Text. Wenn dieser Text aus einer Datei stammen soll, ist der Anrufer dafür verantwortlich. Beim Testen können Sie dann einfach einen Textblock übergeben. Wenn Sie es mit Laufzeitcode verwenden, wird zuerst die Datei gelesen, dann wird diese Funktion aufgerufen. Das ist eine echte Trennung der Bedenken.

Das Prinzip der Einzelverantwortung besagt, dass eine Klasse sich um eine einzelne Funktionalität kümmern sollte, und diese Funktionalität sollte angemessen darin gekapselt sein.

Was genau macht Ihre Methode? Es wird das letzte Auftreten eines Schlüsselworts angezeigt. Jede Zeile innerhalb der Methode arbeitet darauf hin und hat nichts mit etwas anderem zu tun. Das Endergebnis ist nur eins und eins für sich. Mit anderen Worten: Sie müssen diese Methode nicht in etwas anderes aufteilen.

Die Hauptidee hinter dem Prinzip ist, dass Sie am Ende nicht mehr als eine Sache tun sollten. Vielleicht öffnen Sie die Datei und lassen sie auch so, damit andere Methoden sie verwenden können. Sie werden zwei Dinge tun. Oder wenn Sie die mit dieser Methode verbundenen Daten beibehalten möchten, noch einmal zwei Dinge.

Jetzt können Sie die Zeile "Datei öffnen" extrahieren und die Methode ein Dateiobjekt empfangen lassen, mit dem gearbeitet werden soll. Dies ist jedoch eher ein technisches Refactoring als der Versuch, die SRP einzuhalten.

Dies ist ein gutes Beispiel für Überentwicklung. Denken Sie nicht zu viel nach, sonst erhalten Sie eine Reihe einzeiliger Methoden.

Meine Meinung dazu: Es kommt darauf an :-)

Meiner Meinung nach sollte der Code diese Ziele erfüllen, geordnet nach Priorität:

1. Erfüllen Sie alle Anforderungen (dh es macht richtig, was es sollte)
2. Seien Sie lesbar und leicht zu verstehen
3. Seien Sie einfach zu überarbeiten
4. Befolgen Sie die guten Codierungspraktiken / -prinzipien

Für mich besteht Ihr ursprüngliches Beispiel alle diese Ziele (außer vielleicht die Richtigkeit aufgrund der line_number = linebereits in den Kommentaren erwähnten Sache , aber das ist hier nicht der Punkt).

Die Sache ist, SRP ist nicht das einzige Prinzip, dem man folgen muss. Es gibt auch Sie werden es nicht brauchen (YAGNI) (unter vielen anderen). Wenn die Prinzipien kollidieren, müssen Sie sie ausgleichen.

Ihr erstes Beispiel ist perfekt lesbar, kann bei Bedarf leicht umgestaltet werden, folgt jedoch möglicherweise nicht so stark SRP wie es könnte.

Jede Methode in Ihrem dritten Beispiel ist ebenfalls perfekt lesbar, aber das Ganze ist nicht mehr so ​​einfach zu verstehen, da Sie alle Teile in Ihrem Kopf zusammenfügen müssen. Es folgt jedoch SRP.

Wie Sie nichts bekommen jetzt von Splitting Ihre Methode, tun es nicht, weil Sie eine Alternative , die einfacher zu verstehen ist.

Wenn sich Ihre Anforderungen ändern, können Sie die Methode entsprechend umgestalten. Tatsächlich ist das "Alles in einem" möglicherweise einfacher zu überarbeiten: Stellen Sie sich vor, Sie möchten die letzte Zeile finden, die einem beliebigen Kriterium entspricht. Jetzt müssen Sie nur noch eine Prädikat-Lambda-Funktion übergeben, um zu bewerten, ob eine Linie dem Kriterium entspricht oder nicht.

def get_last_match(file, predicate):
    with open(file, 'r') as file:
        line_number = 0
        for line in file:
            if predicate matches line:
                line_number = line
        return line_number

In Ihrem letzten Beispiel müssen Sie das Prädikat 3 Ebenen tief übergeben, dh 3 Methoden ändern, um das Verhalten der letzten zu ändern.

Beachten Sie, dass selbst das Aufteilen des Auslesens der Datei (ein Refactoring, das normalerweise viele als nützlich erachten, einschließlich mir) unerwartete Konsequenzen haben kann: Sie müssen die gesamte Datei in den Speicher lesen, um sie als Zeichenfolge an Ihre Methode zu übergeben. Wenn die Dateien groß sind, entspricht dies möglicherweise nicht Ihren Wünschen.

Fazit: Prinzipien sollten niemals bis zum Äußersten befolgt werden, ohne einen Schritt zurückzutreten und alle anderen Faktoren zu berücksichtigen.

Vielleicht könnte "vorzeitige Aufteilung von Methoden" als Sonderfall vorzeitiger Optimierung angesehen werden ? ;-);

Dies ist wie eine ausgleichende Frage in meinem Kopf ohne einfache richtige und falsche Antwort. Ich werde einfach meine persönlichen Erfahrungen hier teilen, einschließlich meiner eigenen Tendenzen und Fehler während meiner Karriere. YMMV erheblich.

Als Einschränkung arbeite ich in Bereichen, die einige sehr große Codebasen beinhalten (Millionen von LOC, manchmal jahrzehntelanges Erbe). Ich arbeite auch in einem besonders besonderen Bereich, in dem kein Kommentar oder keine Klarheit des Codes zwangsläufig dazu führen kann, dass ein kompetenter Entwickler verstehen kann, was die Implementierung tut (wir können nicht unbedingt einen anständigen Entwickler nehmen und ihn dazu bringen, die Implementierung eines Zustands zu verstehen Implementierung der Fluiddynamik auf dem neuesten Stand der Technik basierend auf einem vor 6 Monaten veröffentlichten Artikel, ohne dass er viel Zeit außerhalb des Codes damit verbringt, sich auf diesen Bereich zu spezialisieren. Dies bedeutet im Allgemeinen, dass nur wenige Entwickler einen bestimmten Teil der Codebasis effektiv verstehen und pflegen können.

Aufgrund meiner besonderen Erfahrungen und vielleicht in Verbindung mit der Besonderheit dieser Branche fand ich es nicht mehr produktiv, SoC, DRY zu verwenden, um Funktionsimplementierungen so lesbar wie möglich zu machen und sogar die Wiederverwendbarkeit bis an die äußersten Grenzen zugunsten von YAGNI, Entkopplung, Testbarkeit, Schreiben von Tests, Dokumentation der Benutzeroberfläche (damit wir zumindest wissen, wie eine Schnittstelle verwendet wird, auch wenn die Implementierung zu viel Fachwissen erfordert) und letztendlich Versand der Software.

Legoblöcke

Eigentlich war ich zu einem früheren Zeitpunkt meiner Karriere geneigt, in die entgegengesetzte Richtung zu gehen. Ich war so begeistert von funktionaler Programmierung und Richtlinienklassenentwürfen in Modern C ++ Design und Vorlagen-Metaprogrammierung und so weiter. Insbesondere war ich von den kompaktesten und orthogonalsten Designs begeistert, bei denen Sie all diese kleinen Funktionen (wie "Atome") haben, die Sie auf scheinbar unendliche Weise miteinander kombinieren (um "Moleküle" zu bilden), um die gewünschten Ergebnisse zu erzielen. Ich wollte fast alles als Funktionen schreiben, die aus wenigen Codezeilen bestanden, und an einer so kurzen Funktion ist nicht unbedingt etwas von Natur aus falsch (sie kann immer noch sehr weit anwendbar sein und Code klarstellen). außer dass ich anfing, in die dogmatische Richtung zu gehen und zu denken, mein Code habe etwas falsch gemacht, wenn eine Funktion mehr als ein paar Zeilen umfasste. Und ich habe ein paar wirklich nette Spielsachen und sogar einen Produktionscode aus dieser Art von Code herausgeholt, aber ich habe die Uhr ignoriert: die Stunden, Tage und Wochen, die vergehen.

Insbesondere während ich die Einfachheit jedes kleinen "Legoblocks" bewunderte, den ich auf unendliche Weise kombinieren konnte, ignorierte ich die Zeit und die geistige Kraft, die ich in das Zusammensetzen all dieser Blöcke steckte, um eine ausgeklügelte "Erfindung" zu bilden. Darüber hinaus ignorierte ich in den seltenen, aber schmerzhaften Fällen, in denen etwas mit der aufwändigen Erfindung schief gelaufen war, absichtlich die Zeit, die ich damit verbrachte, herauszufinden, was schief gelaufen war, indem ich eine scheinbar endlose Reihe von Funktionsaufrufen verfolgte, die jedes einzelne dezentrale Legostück und jede Untergruppe von analysierten ihre Kombinationen, wenn das Ganze viel einfacher gewesen wäre, wenn es nicht aus diesen "Legos" gemacht worden wäre, wenn man so will, und nur als eine Handvoll fleischigerer Funktionen oder als mittelschwere Klasse geschrieben.

Trotzdem schloss sich der Kreis und als die Fristen mich zwangen, mir der Zeit bewusster zu werden, wurde mir klar, dass meine Bemühungen mich mehr darüber lehrten, was ich falsch machte als was ich richtig machte . Ich begann hier und da wieder die fleischigere Funktion und das Objekt / die Komponente zu schätzen, dass es pragmatischere Möglichkeiten gibt, ein angemessenes Maß an SoC zu erreichen, wie David Arnodurch Trennen der Dateieingabe von der Zeichenfolgenverarbeitung gezeigt wird, ohne dass die Zeichenfolgenverarbeitung notwendigerweise bis zum Äußersten zerlegt wird körnige Ebene vorstellbar.

Fleischigere Funktionen

Und noch mehr, ich fing an, mit einigen Code-Duplikationen, sogar einigen logischen Duplikationen, einverstanden zu sein (ich sage nicht Copy and Paste-Codierung, alles, worüber ich spreche, ist "Balance" zu finden), vorausgesetzt , die Funktion ist nicht anfällig wiederholte Änderungen vorzunehmen und in Bezug auf seine Verwendung dokumentiert und vor allem gut getestet, um sicherzustellen, dass seine Funktionalität korrekt mit der Dokumentation übereinstimmt und dies auch bleibt. Mir wurde klar, dass Wiederverwendbarkeit weitgehend mit Zuverlässigkeit verbunden ist .

Mir ist klar geworden, dass selbst die fleischigste Funktion, die immer noch einzigartig genug ist, um nicht zu eng anwendbar und zu klobig zu sein, um sie zu verwenden und zu testen, selbst wenn sie eine Logik in einigen entfernten Funktionen an anderer Stelle in der Codebasis dupliziert und vorausgesetzt, dass sie vorhanden ist Gut getestet und zuverlässig, und die Tests stellen vernünftigerweise sicher, dass dies auch so bleibt. Sie sind der am meisten zerlegten und flexiblen Kombination von Funktionen vorzuziehen, denen diese Qualität fehlt. Ich mag einige der fleischigeren Sachen heutzutage gut genug, wenn sie zuverlässig sind .

Es scheint mir auch , dass die meisten der Zeit, es ist billiger zu realisieren Sie sind Gonna Bedürfnis , etwas im Nachhinein und fügen Sie es, sofern der Code zumindest empfänglich ohne Kaskadierung Höllenfeuer zu neuen Ergänzungen ist, als auf Code alle möglichen Dinge , wenn Sie aren Ich werde es nicht brauchen und mich dann der Versuchung stellen, alles zu entfernen, wenn es anfängt, eine echte PITA zu werden, die es zu pflegen gilt.

Das habe ich also gelernt. Dies sind die Lektionen, die ich für notwendig erachtet habe, um sie im Nachhinein in diesem Zusammenhang persönlich zu lernen, und als Einschränkung sollte sie mit einem Körnchen Salz eingenommen werden. YMMV. Aber hoffentlich kann dies für Sie von Nutzen sein, wenn es darum geht, die richtige Balance zu finden, um Produkte zu versenden, die Ihre Benutzer innerhalb eines angemessenen Zeitraums glücklich machen und sie effektiv warten.

Das Problem, auf das Sie stoßen, ist, dass Sie Ihre Funktionen nicht in ihrer am wenigsten reduzierten Form faktorisieren. Schauen Sie sich Folgendes an: (Ich bin kein Python-Programmierer, also lassen Sie mich etwas locker)

def lines_from_file(file):
    with open(file, 'r') as text:
        line_number = 1
        lines = []
        for line in text:
            lines.append((line_number, line.strip()))
            line_number += 1
    return lines

def filter(l, func):
    new_l = []
    for x in l:
        if func(x):
            new_l.append(x)
    return new_l

def contains(needle):
    return lambda haystack: needle in haystack

def last(l):
    length = len(l)
    if length > 0:
        return l[length - 1]
    else:
        return None

Jede der oben genannten Funktionen hat etwas völlig anderes, und ich glaube, es würde Ihnen schwer fallen, diese Funktionen weiter zu berücksichtigen. Wir können diese Funktionen kombinieren, um die anstehende Aufgabe zu erfüllen.

lines = lines_from_file('./test_file')
filtered = filter(lines, lambda x : contains('some value')(x[1]))
line = last(filtered)
if line is not None:
    print(line[0])

Die obigen Codezeilen können leicht zu einer einzigen Funktion zusammengefasst werden, um genau das auszuführen, was Sie tun möchten. Der Weg, um Probleme wirklich zu trennen, besteht darin, komplexe Vorgänge in ihre am meisten berücksichtigte Form zu zerlegen. Sobald Sie eine Gruppe gut faktorisierter Funktionen haben, können Sie diese zusammensetzen, um das komplexere Problem zu lösen. Eine schöne Sache bei gut berücksichtigten Funktionen ist, dass sie häufig außerhalb des Kontextes des aktuellen Problems wiederverwendbar sind.

Ich würde sagen, dass es in der Tat nie zu viele Trennung von Bedenken gibt. Es kann jedoch Funktionen geben, die Sie nur einmal verwenden und nicht einmal separat testen. Sie können sicher eingefügt werden , damit die Trennung nicht in den äußeren Namespace eindringt .

Ihr Beispiel wird buchstäblich nicht benötigt check_if_keyword_in_string, da die Zeichenfolgenklasse bereits eine Implementierung bereitstellt: keyword in lineist ausreichend. Möglicherweise möchten Sie jedoch Implementierungen austauschen, z. B. eine, die eine Boyer-Moore-Suche verwendet, oder eine verzögerte Suche in einem Generator zulassen. dann würde es Sinn machen.

Sie find_last_appearance_of_keywordkönnten allgemeiner sein und das letzte Erscheinungsbild eines Elements in einer Sequenz finden. Dazu können Sie eine vorhandene Implementierung verwenden oder eine wiederverwendbare Implementierung vornehmen. Es kann auch ein anderer Filter verwendet werden , sodass Sie nach einem regulären Ausdruck oder nach Übereinstimmungen ohne Berücksichtigung der Groß- und Kleinschreibung usw. suchen können.

Normalerweise verdient alles, was sich mit E / A befasst, eine separate Funktion. Daher ist es get_text_from_filemöglicherweise eine gute Idee, wenn Sie verschiedene Sonderfälle direkt behandeln möchten. Es kann sein, dass Sie sich nicht auf einen äußeren IOErrorHandler verlassen.

Sogar das Zählen von Zeilen kann ein separates Problem sein, wenn Sie in Zukunft möglicherweise z. B. Fortsetzungszeilen (z. B. mit \) unterstützen müssen und die logische Zeilennummer benötigen würden. Oder Sie müssen möglicherweise Kommentarzeilen ignorieren, ohne die Zeilennummerierung zu unterbrechen.

Erwägen:

def get_last_appearance_of_keyword(filename, keyword):
    with open(filename) as f:  # File-opening concern.
        numbered_lines = enumerate(f, start=1)  # Line-numbering concern.
        last_line = None  # Also a concern! Some e.g. prefer -1.
        for line_number, line in numbered_lines:  # The searching concern.
            if keyword in line: # The matching concern, applied.
                last_line = line_number
    # Here the file closes; an I/O concern again.
    return last_line

Sehen Sie, wie Sie Ihren Code möglicherweise aufteilen möchten , wenn Sie Bedenken haben, die sich in Zukunft ändern könnten, oder nur, weil Sie feststellen, wie derselbe Code an anderer Stelle wiederverwendet werden kann.

Dies sollten Sie beachten, wenn Sie die ursprüngliche kurze und süße Funktion schreiben. Auch wenn Sie die Bedenken noch nicht als Funktionen getrennt haben müssen, halten Sie sie so weit wie möglich getrennt. Es hilft nicht nur, den Code später weiterzuentwickeln, sondern auch, den Code sofort besser zu verstehen und weniger Fehler zu machen.

Wann ist es "zu viel" Trennung? Noch nie. Sie können nicht zu viel Trennung haben.

Ihr letztes Beispiel ist ziemlich gut, aber Sie könnten vielleicht die for-Schleife mit a text.GetLines(i=>i.containsKeyword)oder so vereinfachen .

* Praktische Version: Stoppen Sie, wenn es funktioniert. Trennen Sie mehr, wenn es bricht.