Scheiding van zorgen: wanneer is het "te veel" scheiding?

Ik hou echt van schone code en ik wil mijn code altijd op de best mogelijke manier coderen. Maar er was altijd één ding, ik begreep het niet echt:

Wanneer is het teveel van "separatie van zorgen" met betrekking tot methoden?

Laten we zeggen dat we de volgende methode hebben:

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

Ik denk dat deze methode prima is zoals hij is. Het is eenvoudig, gemakkelijk te lezen en het is duidelijk wat de naam zegt. Maar: het doet niet echt "slechts één ding". Het opent het bestand en vindt het vervolgens. Dat betekent dat ik het nog verder zou kunnen opsplitsen (ook gezien het "Single Responsibility Principle"):

Variatie B (Nou, dit is op de een of andere manier zinvol.) Op deze manier kunnen we gemakkelijk het algoritme hergebruiken om de laatste verschijning van een sleutelwoord in een tekst te vinden, maar het lijkt "te veel". Ik kan niet uitleggen waarom, maar ik voel gewoon " "het op die manier):

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

Variatie C (Dit is naar mijn mening gewoon absurd.) We zijn in feite een one-liner om te zetten naar een andere methode met slechts één regel tweemaal, maar je zou kunnen beweren dat de manier om iets te openen in de toekomst kan veranderen vanwege sommige functieverzoeken en omdat we het niet vaak willen veranderen, maar slechts één keer, we kapselen het in en scheiden we onze hoofdfunctie nog verder):

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

Dus mijn vraag nu: wat is de juiste manier om deze code te schrijven en waarom zijn de andere benaderingen goed of fout? Ik heb altijd geleerd: scheiding, maar nooit wanneer het gewoon te veel is. En hoe kan ik er zeker van zijn dat het in de toekomst "precies goed" is en dat er niet meer scheiding nodig is als ik opnieuw codeer?

6
toegevoegd de auteur gnat, de bron
In uw laatste voorbeeld implementeert u twee bestaande functies opnieuw: open en in . Het opnieuw implementeren van bestaande functies verhoogt de scheiding van zorgen niet, de zorg is al verwerkt in de bestaande functie!
toegevoegd de auteur user35925, de bron
Terzijde: bent u van plan een string of een cijfer terug te geven? regelnummer = 0 is een numerieke standaardwaarde, en regelnummer = regel wijst een tekenreekswaarde toe (dit is de regel inhoud niet de positie )
toegevoegd de auteur Caleth, de bron

6 antwoord

Uw verschillende voorbeelden van het splitsen van problemen in afzonderlijke functies hebben allemaal hetzelfde probleem: u codeert de bestandsafhankelijkheid nog steeds hard naar get_last_appearance_of_keyword . Hierdoor is die functie moeilijk te testen omdat deze nu moet antwoorden op een bestand dat bestaat in het bestandssysteem wanneer de test wordt uitgevoerd. Dit leidt tot broze testen.

Dus ik zou gewoon je originele functie veranderen 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

Nu hebt u een functie die slechts één verantwoordelijkheid heeft: zoek de laatste instantie van een trefwoord in een tekst. Als die tekst uit een bestand moet komen, wordt dat de verantwoordelijkheid van de beller om mee om te gaan. Tijdens het testen kunt u dan gewoon een tekstblok doorgeven. Als u het gebruikt met runtime-code, wordt eerst het bestand gelezen en vervolgens wordt deze functie aangeroepen. Dat is echte scheiding van zorgen.

6
toegevoegd
Denk aan niet-hoofdlettergevoelig zoeken. Denk na over het overslaan van commentaarregels. De scheiding van zorgen kan anders worden. Ook is regelnummer = regel duidelijk een vergissing.
toegevoegd de auteur Rorick, de bron
ook het laatste voorbeeld doet dit vrijwel
toegevoegd de auteur Ewan, de bron

Wanneer is het "te veel" scheiding? Nooit. Je kunt niet te veel scheiding hebben.

Your last example is pretty good, but you could maybe simplify the for loop with a text.GetLines(i=>i.containsKeyword) or something.

* Praktische versie: stop wanneer het werkt. Scheid meer wanneer het breekt.

4
toegevoegd
@cariehl je zou een antwoord moeten toevoegen waarin je die zaak bepleit. Ik denk dat je zou merken dat om het echt te laten werken, je in die functies wat meer logica nodig zou hebben
toegevoegd de auteur Ewan, de bron
toegevoegd de auteur Ewan, de bron
"Je kunt niet te veel scheiding hebben." Ik denk niet dat dit waar is. OP's derde voorbeeld is alleen het herschrijven van algemene Python-constructies in afzonderlijke functies. Heb ik echt een hele nieuwe functie nodig om 'als x in y' uit te voeren?
toegevoegd de auteur Chthonic One, de bron

Ik zou zeggen dat er inderdaad nooit een te grote scheiding van zorgen is. Maar er kunnen functies zijn die u slechts één keer gebruikt en zelfs niet afzonderlijk testen. Ze kunnen veilig inline worden geplaatst, om te voorkomen dat de scheiding in de buitenste naamruimte sijpelt.

Uw voorbeeld heeft letterlijk check_if_keyword_in_string niet nodig, omdat de stringklasse al een implementatie biedt: trefwoord in regel is voldoende. Maar u kunt van plan zijn om implementaties om te wisselen, bijvoorbeeld naar een met behulp van een Boyer-Moore-zoekopdracht of een lui onderzoek in een generator toestaat; dan zou het logisch zijn.

Uw find_last_appearance_of_keyword kan algemener zijn en het laatste voorkomen van een item in een reeks vinden. Daarvoor kunt u een bestaande implementatie gebruiken of een herbruikbare implementatie maken. Het kan ook een ander filter bevatten, zodat u kunt zoeken naar een regex, of voor niet-hoofdlettergevoelige matches, enz.

Gewoonlijk verdient alles wat met I/O te maken heeft een aparte functie, dus get_text_from_file kan een goed idee zijn als je verschillende speciale gevallen regelrecht wilt afhandelen. Dit is misschien niet het geval als u daarvoor een externe IOError -hulpprogramma gebruikt.

Zelfs lijntelling kan een aparte zorg zijn als u in de toekomst wellicht ondersteuning nodig hebt voor bijvoorbeeld vervolglijnen (bijvoorbeeld met \ ) en zou het logische regelnummer nodig hebben. Of u moet misschien commentaarregels negeren, zonder de regelnummering te verbreken.

Overwegen:

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

Bekijk hoe u wilt om uw code te splitsen wanneer u bepaalde zorgen overweegt die in de toekomst kunnen veranderen, of alleen omdat u merkt dat dezelfde code elders kan worden hergebruikt.

Dit is iets om op te letten wanneer je de originele korte en zoete functie schrijft. Zelfs als u de zorgen die als functies zijn gescheiden nog niet nodig hebt, houd ze dan zo veel als praktisch mogelijk. Het helpt niet alleen om de code later te ontwikkelen, maar helpt ook om de code meteen beter te begrijpen en minder fouten te maken.

2
toegevoegd

Het probleem dat u tegenkomt, is dat u uw functies niet in de meest gereduceerde vorm invoert. Bekijk het volgende: (ik ben geen python-programmeur, dus snijd me wat slappe kost)

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

Elk van de bovenstaande functies doet iets heel anders, en ik geloof dat je het moeilijk zou hebben om die functies verder te integreren. We kunnen die functies combineren om de taak te volbrengen.

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])

De bovenstaande coderegels kunnen gemakkelijk in één functie worden samengevoegd om precies datgene te doen wat u wilt doen. De manier om problemen echt van elkaar te scheiden, is door complexe operaties in hun meest gefactureerde vorm te splitsen. Als u eenmaal een groep goed gefactureerde functies hebt, kunt u beginnen ze samen te stellen om het complexere probleem op te lossen. Een aardig ding over goed gefactureerde functies, is dat ze vaak herbruikbaar zijn buiten de context van het huidige probleem bij de hand.

1
toegevoegd

Het principe van de enkele verantwoordelijkheid stelt dat een klasse voor één enkel stuk functionaliteit moet zorgen en dat deze functionaliteit op de juiste manier in de inhoud moet worden ingekapseld.

Wat doet uw methode precies? Het krijgt de laatste weergave van een sleutelwoord. Elke regel binnen de methode werkt hier naar toe en het is niet gerelateerd aan iets anders, en het eindresultaat is slechts een en een alleen. Met andere woorden: u hoeft deze methode niet in iets anders te splitsen.

Het belangrijkste idee achter het principe is dat je niet meer dan één ding op het einde moet doen. Misschien open je het bestand en laat het ook zo staan ​​zodat andere methoden het kunnen gebruiken, je zult twee dingen doen. Of als je de gegevens met betrekking tot deze methode zou blijven herhalen, nogmaals, twee dingen.

U kunt nu de regel "open bestand" extraheren en ervoor zorgen dat de methode een bestandsobject ontvangt om mee te werken, maar dat is meer een technische refactoring dan proberen te voldoen aan de SRP.

Dit is een goed voorbeeld van over-engineering. Denk niet te veel na of je krijgt een aantal methoden met één lijn.

1
toegevoegd
@JoshuaJones Er is niets inherent verkeerd met functies met één regel, maar ze kunnen een belemmering vormen als ze niet iets nuttigs abstraheren. Een one-line functie om de cartesiaanse afstand tussen twee punten te retourneren is erg handig, maar als je een one-liner hebt voor return keyword in text , dan is dat gewoon een onnodige laag bovenop een ingebouwde in taalconstructie.
toegevoegd de auteur Chthonic One, de bron
@JoshuaJones In dat verband abstraheer je iets nuttigs. In de context van het oorspronkelijke voorbeeld is er geen goede reden om zo'n functie te hebben. in is een veelvoorkomend Python-zoekwoord, het bereikt het doel en het is expressief op zichzelf. Het schrijven van een wrapper-functie eromheen alleen al omwille van het feit dat een wrapper-functie de code verduistert, maakt het minder direct intuïtief.
toegevoegd de auteur Chthonic One, de bron
Er is absoluut niets mis met functies met één regel. Sommige van de nuttigste functies zijn zelfs maar één regel code.
toegevoegd de auteur dfdffewfw, de bron
@cariehl Waarom zou zoekwoord in tekst een onnodige laag zijn? Als je merkt dat je consequent die code gebruikt in een lambda als een parameter in hogere orde functies, waarom zou je het dan niet in een functie verpakken?
toegevoegd de auteur dfdffewfw, de bron

Mijn kijk erop: het hangt ervan af :-)

Naar mijn mening moet code deze doelen bereiken, gesorteerd op prioriteit:

  1. Voldoen aan alle vereisten (dat wil zeggen dat het correct doet wat het zou moeten doen)
  2. Wees leesbaar en gemakkelijk te volgen/begrijpen
  3. Wees gemakkelijk te refactiveren
  4. Volg goede codeermethoden/-principes

Voor mij geeft je oorspronkelijke voorbeeld al deze doelen door (behalve misschien de juistheid vanwege het regelnummer = regel ding dat al is genoemd in de reacties , maar daar gaat het hier niet om).

Het punt is dat SRP niet het enige principe is om te volgen. Er is ook U bent het niet nodig (YAGNI) (naast vele anderen). Wanneer de principes botsen, moet je ze in evenwicht brengen.

Uw eerste voorbeeld is perfect leesbaar, gemakkelijk te refactiveren wanneer dat nodig is, maar volgt SRP mogelijk niet zo goed als het zou kunnen.

Elke methode in je derde voorbeeld is ook perfect leesbaar, maar het geheel is niet meer zo gemakkelijk te begrijpen omdat je alle stukjes in je hoofd moet stoppen. Het volgt echter wel SRP.

Aangezien u op dit moment niks krijgt van het splitsen van uw methode, doe het niet, want u hebt een beter begrijpbaar alternatief.

Naarmate uw vereisten veranderen, kunt u de methode dienovereenkomstig aanpassen. In feite kan het "alles in één ding" eenvoudiger zijn om te refactoren: stel je voor dat je de laatste regel wilt vinden die overeenkomt met een willekeurig criterium. Nu moet je gewoon wat predikaat lambda-functie doorgeven om te evalueren of een regel overeenkomt met het criterium of niet.

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 je laatste voorbeeld moet je het predikaat 3 niveaus diep doorgeven, d.w.z. 3 methoden aanpassen om het gedrag van de laatste te wijzigen.

Merk op dat zelfs het opsplitsen van de uitlezing van het bestand (een refactoring die meestal veel nuttig lijkt, waaronder ikzelf) onverwachte gevolgen kan hebben: je moet het hele bestand in het geheugen lezen om het door te geven als een tekenreeks naar je methode. Als de bestanden groot zijn, is dat misschien niet wat u zoekt.

Kortom: principes mogen nooit tot het uiterste worden gevolgd zonder een stap terug te doen en rekening te houden met alle andere factoren.

Misschien kan "voortijdige splitsing van methoden" worden beschouwd als een speciaal geval van voortijdige optimalisatie ? ;-)

0
toegevoegd