Stil Frage

[tom1968]

Ich habe eine Stil Frage, bzw. was erfahrenere Programmierer empfehlen würden:

Vorweg um was es geht: Die folgende Methode soll mir anhand einer Video Liste Datensätze herausfiltern deren Laufzeitlänge länger als der Inhalt des Arguments "Wert" ist. Es geht dabei nur um die erste Stelle(n).
Das Format von "Wert" beträgt beispielsweise "28:57", also 28 Minuten und 57 Sekunden
Gespeichert werden soll dann ein "x" in self.index[] wenn das Ergebnis wahr ist, ansonsten "n".

Mein erster und funktionierender Entwurf war dieser hier:

Code: Alles auswählen

class Videotabelle(Tabelle):
....
....
    def find_greater_as(self,wert):
        counter = 0
        for i in range(len(self.index)):
	    zahl = self.tabelle[i][9].split(':')
            zahl = int(zahl[0])
            if  zahl >= wert:
                self.index[i] ="x"
		counter +=1
	    else:
		self.index[i] ="n"
        return counter

Das ist natürlich langatmig und umständlich, etwas eleganter:

Code: Alles auswählen

class Videotabelle(Tabelle):
....
....
    def find_greater_as(self,wert):
        counter = 0        
        for i in range(len(self.index)):
            self.index[i] = "x" if int(self.tabelle[i][9].split(':')[0]) >= wert else "n" # Zeile*
            if self.index[i] == "x":
                counter +=1
	return counter

Besser dürfte vermutlich Variante 2 sein, aber auch schwerer zu lesen bzw. Monate später wieder zu verstehen ?!
Was sollte da Vorrang haben, die Kürze des Codes oder die Verständlichkeit irgendwann später mal ?

... Und gibt es eine Möglichkeit in diese Zeile "...#Zeile*" noch den Counter mit einzubauen ?

Der Versuch das so zu machen:

Code: Alles auswählen

counter = 0
self.index[i] = "x"; counter +=1 if int(self.tabelle[i][9].split(':')[0]) >= wert else "n"

ergab die Fehlermeldung: TypeError: unsupported operand type(s) for +=: 'int' and 'str'
Wenn ich statt des Semilikons ein Komma oder Doppelpunkt verwende bekomme ich einen Syntax Error

MfG
Tom

[Hyperion]

Also erst einmal kann man in Python *direkt* über Datenstrukturen iterieren:

Code: Alles auswählen

# schlecht (Anit-Pattern!):
for i in range(container):
    # do somethind with container[i]

#besser:
for item in container:
    # do something with item

Wenn Du den Index brauchst, dann kannst Du Dir mit ``enumerate`` einen erzeugen:

Code: Alles auswählen

for index, item in enumerate(container).
    # index geht von 0 bis len(container) - 1

Ich denke damit kann man das alles schon lesbarer formulieren!

Zudem finde ich die Semantik der Methode komisch. Zum einen manipulierst Du interne Datenstrukturen (self.index), zum anderen zählst Du Ergebnisse und gibst diese Anzahl zurück; so etwas ist eher ungewöhnlich! Die Anzahl kannst Du doch locker außerhalb der Funktion herausfinden: ``len(filter(lambda c: c == "x", index))``
oder gar so:

Code: Alles auswählen

index = ['x', 'n', 'n', 'x']
from collections import Counter
c = Counter(index)
c["x"]
> 2

Wenn es da keinen Grund für gibt, würde ich beides trennen, da das eine inhaltlich mit dem anderen nichts zu tun hat. Eine Funktion sollte *eine* Aufgabe erledigen; nicht zwei.

Wenn es denn aus Performancegründen sinnvoll ist, das Zählen während des Abarbeitens zu erledigen, so würde ich *diesen* Wert intern speichern und nach dem Aufruf der Methode separat abfragen.

Die Namen ``tabelle`` und ``index`` können natürlich im Kontext gut sein - auf den ersten Blick ohne Kontextinfos sind sie ziemlich nichtssagend! Die "tabelle" scheint ja eine Liste von Listen zu sein - aber was wird denn in diesen gespeichert? Das lässt sich doch sicher benennen und dann hast Du schon einen besseren Namen

Da es irgend wie um Filme geht, scheinen da ja Filmdaten drin zu stehen!?! Dann könnte man sie schlicht ``films`` nennen

Abschließend zu Deiner Frage: Ich würde vermutlich eine kleine Funktion schreiben, die mir den 9. Index aus einem (Tabellen-)Item herausholt, evtl. einfach auch über einen ``itemgetter``, den man aussagekräftig benennen kann. Wenn Du dann noch über die Items als solche iterierst, bleibt da nicht mehr so viel unlesbarer Code übrig.

[Sirius3]

Wie schon im anderen Thread von Dir mehrfach betont, würde man solche Tabellen in Python völlig anders lösen. Zumindest solltest Du statt Listen von Listen den Einträgen in »self.tabelle« eine eigene Klasse gönnen (ein named tuple böte sich an).
Eine Index-Liste ist ungewöhnlich, das heißt meistens zu kompliziert und komplex oder überflüssig.
Statt 'n' oder 'x' gibt es in Python »False« und »True«.
Zahlen (Filmlänge) werden beim Einlesen in eine Zahl umgewandelt und bei der Ausgabe wieder in einen String zurück und nicht jedesmal bei der Verarbeitung eines Datensatzes.

Stil, der auch nach einigen Monaten noch verständlich ist, sähe so aus:

Code: Alles auswählen

class Videotabelle(Tabelle):
    @classmethod
    def film_duration(film_entry):
        hours, minutes = film_entry[9].split(':')
        return int(hours)+int(minutes)/60.0
    
    def find_greater_as(self, min_duration):
        self.index = ["nx"[self.film_duration(film_entry)>=min_duration]
                        for film_entry in self.tabelle]
        
    def get_x_count(self):
        return self.index.count('x')

besser

Code: Alles auswählen

def durationstring_to_float(duration):
    hours, minutes = duration.split(':')
    return int(hours)+int(minutes)/60.0

class Film:
    def __init__(self, name, …, duration):
        self.name = name
        self.duration = durationstring_to_float(duration)

class Videotabelle(Tabelle):
    def find_greater_as(self, min_duration):
        self.index = [film.duration>=min_duration for film in self.tabelle]
        
    def get_x_count(self):
        return self.index.count(True)

[snafu]

@Sirius3: Meintest du im ersten Quelltext nicht eher `@staticmethod` anstatt `@classmethod`?

[Sirius3]

@snafu: Du hast recht.

[tom1968]

Erstmal recht herzlichen Dank euch beiden Hyperion und Sirius3....

@Hyperion

Also erst einmal kann man in Python *direkt* über Datenstrukturen iterieren: ....

Ja, das versuchte ich zuerst auch, aber es klappe nicht (hatte vergessen die zweite Variable (item) anzugeben )
Ich hab das nun geändert, habe dazu aber noch ein Frage:
Rein vom Code her ist das in diesem Fall mit dem Iteritieren ja nicht kürzer – und für mich auch (noch) nicht lesbarer oder intuitiv verständlicher.

Code: Alles auswählen

for i, item in enumerate(self.index):	   # neue Version

for i in range(self.index):   # alte Version

Erfordern for´s mit 'range' mehr Rechenzeit und/oder Speicher oder spricht in so einem Fall (wie diesem) rein der pythonsche Stil bzw. die Lesbarkeit für andere eine Rolle ?

Zudem finde ich die Semantik der Methode komisch. Zum einen manipulierst Du interne Datenstrukturen (self.index), zum anderen zählst Du Ergebnisse und …..

Fand ich anfangs eigentlich nicht und machte das sehr bewußt so, denn diese Funktion selektiert gesuchte Datensätze und gibt dann auch noch die Summe derer zurück, was ja schon miteinander zu tun hat.
Aber ich denke Du hast recht, in sich logischer ist es wenn man es trennt, was ich nun auch tat.

Code: Alles auswählen

class Videotabelle(Tabelle):
    def find_greater(self,wert):
        for i, item in enumerate(self.index):
            self.index[i] = "x" if int(self.tabelle[i][9].split(':')[0]) >= wert else "n"

class Fenster(Tk):
....
....
        elif eingabe[0] == ">":
            tabelle.find_greater_as(int(eingabe[1:]))
	    self.info.config(text = str(tabelle.index.count('x')) + " Datensätze gefunden")
	    self.ausgabe(tabelle.get_first_row())
....

Die vorletzte Zeile mit der Ausgabe der gefundenen Datensätze könnte man vielleicht noch in eine extra Methode packen, denn ich habe insgesammt 11 solcher Suchfunktionen.... ?

Das mit 'tabelle' vs 'films' stimmt natürlich – zu meiner Schande muss ich eingestehen dass mir bisher nur noch längere Namen einfielen - wie 'videotabelle' – was unpraktisch wäre.

@Sirius3

Wie schon im anderen Thread von Dir mehrfach betont, würde man solche Tabellen in Python völlig anders lösen.....

Mag sein, aber ich bin nicht 'man' ….
Im Ernst, ich muss halt irgendwie irgendwo beginnen und hangle mich sozusagen von Ast zu Ast, ausgehend von meinen Programmiergewohnheiten und meinem Wissen nach oben....oder öfter mal seitwärts
Aber ich denke dieses Thema gehört eher in den anderen Thread und wie ich dort schon sagte, ziehe ich das jetzt so durch, kann mir aber sehr gut vorstellen das dann, wenns fertig ist, nochmal mit einem generell besseren Ansatz umzusetzen.

Dennoch: Verstehe ich dich richtig, Du würdest eine weitere Klasse machen in der jeder Datensatz eine Instanz/Objekt darstellt

Code: Alles auswählen

return self.index.count('x') 

Das ist genau das wonach ich suchte, peinlich - denn ich hatte mir dieses .count() vor Wochen sogar schon mal notiert, es aber wieder vergessen

Also, nochmal vielen Dank und versteht es nicht als Undankbarkeit oder Verbohrtheit wenn ich nicht alles was ihr hier vorschlagt sofort umsetze, aber auch meine Zeit um mich damit auseinanderzusetzen ist endlich (momentan leider sogar sehr) und so geht es oft nur in kleinen Schritten voran.

[Leonidas]

Ich hab das nun geändert, habe dazu aber noch ein Frage:
Rein vom Code her ist das in diesem Fall mit dem Iteritieren ja nicht kürzer – und für mich auch (noch) nicht lesbarer oder intuitiv verständlicher.

Code: Alles auswählen

for i, item in enumerate(self.index):	   # neue Version

for i in range(self.index):   # alte Version

Erfordern for´s mit 'range' mehr Rechenzeit und/oder Speicher oder spricht in so einem Fall (wie diesem) rein der pythonsche Stil bzw. die Lesbarkeit für andere eine Rolle ?

Der Unterschied ist, dass du im Schleifenkörper direkt das Element hast, statt es erstmal via Listenzugriff ala ``self.index`` rausziehen zu müssen. Damit wird auch vom Verständnis klar dass du über *Elemente* iterierst und nicht über Zahlen die zufälligerweise mit den Indizes der Elemente in einer anderen Datenstruktur übereinstimmen.

Überlag doch mal intuitiv: über was geht die Schleife? Geht sie über Zahlen? Geht sie über Elemente? Wenn du über Zahlen iterierst dann macht ``range`` natürlich Sinn. Im anderen Fall, wenn du über Elemente iterieren willst, warum erstellst du erstmal Zahlen, iterierst darüber und versuchtst diese Zahlen wieder an die Elemente zurückzukorrelieren. Das ist doch allein sementisch schon Quatsch.

[tom1968]

@Leonidas
Danke für die Erklärung, hab nun ein wenig rumprobiert (daher dauerte es mit der Antwort etwas)...und ja es stimmt schon, zudem kann man erheblich mehr über direktes Iteritieren lösen, als ich Anfangs annahm. Diese "Indexerei" ist halt noch so eine "Altlast" die ich mit mir rumtrage

Bei folgendem, denke ich, komme ich aber nicht um den Index herum ?

Angenommen es wird der Datensatz 50 (von 100) angezeigt und ich will per ">" oder "<" Button einen Datensatz vor oder zurück springen - dann löse ich das momentan noch so:

Code: Alles auswählen

  
    def get_next_row(self):
        if self.zeiger == len(self.tabelle):  
            return self.tabelle[self.zeiger][:]
        for i in range (self.zeiger+1 ,len(self.tabelle)):
            if self.index[i]:
                self.zeiger = i
                return self.tabelle[self.zeiger][:]
        return self.tabelle[self.zeiger][:]
                                                               
    def get_previous_row(self):
        if self.zeiger == 0:
            return self.tabelle[self.zeiger][:]
        for i in range (self.zeiger-1, -1, -1):
            if self.index[i]:
                self.zeiger = i
                return self.tabelle[self.zeiger][:]
        return self.tabelle[self.zeiger][:]

Oder mit Worten: Ausgehend vom 51 Datensatz wird bis zum letzten Datensatz ein Datensaz nach dem anderen überprüft ob der self.index True ist, wenn ja - dann wird der Datensatz zurückgegeben, gleiches bei "<" vom Datensatz 49 bis Datensatz 0.

Was man machen könnte um den Code verständlicher zu machen wäre "i" durch "zeile" zu ersetzen....

MfG
thomas

[BlackJack]

@tom1968: Die `get_next_row()`-Methode ist fehlerhaft weil die erste ``if``-Bedingung nie wahr werden dürfte, beziehungsweise wenn sie es wird, führt die nächste Zeile zwingend zu einem `IndexError`.

[tom1968]

@BlackJack
Stimmt, Du hast recht, es fehlt ein "-1" nach "len(self.tabelle)"
Eigenartig, habs eben am Programm ausprobiert, gibt keine Fehlermeldung - daher fiel mir das bisher nie auf, komisch denn ich fange da nichts mit "try" und "except" ab ... ???

Ich kann sogar +1 eingeben und es passiert nichts ungewöhnliches, muss ich mir nochmal genau ansehen

Edit: Fehler gefunden, da hatte sich am Ende der Tabelle eine Zeile eingemogelt ....die natürlich nie angezeigt wurde weil self.index "False" anzeigt und somit konnte das nicht über das Ende der Tabelle gehen und einen Fehler auslösen.

[Sirius3]

@tom1968: Die »if«-Bedingungen sind in beiden Funktionen unnötig und es stört deshalb auch nicht, dass eine davon fehlerhaft ist. Codewiederholungen solltest Du vermeiden, und »return self.tabelle[self.zeiger][:]« wiederholt sich sehr oft. Mit »get_…« verbindet man normalerweise Methoden, die den Zustand eines Objektes nicht ändern, besser wäre es die Methoden »fetch…« oder ähnlich zu nennen. Dann ergibt sich:

Code: Alles auswählen

    def fetch_next_row(self):
        for i in range (self.zeiger+1 ,len(self.tabelle)):
            if self.index[i]:
                self.zeiger = i
                break
        return self.tabelle[self.zeiger][:]
                                                               
    def fetch_previous_row(self):
        for i in range (self.zeiger-1, -1, -1):
            if self.index[i]:
                self.zeiger = i
                break
        return self.tabelle[self.zeiger][:]

Natürlich kann man jetzt noch die passenden Python-Funktionen zum Durchsuchen von Listen verwenden:

Code: Alles auswählen

    def fetch_next_row(self):
        try:
            self.zeiger = self.index.index(True, self.zeiger+1)
        except ValueError:
            pass
        return self.tabelle[self.zeiger][:]
                                                               
    def fetch_previous_row(self):
        try:
            self.index.reverse()
            self.zeiger = len(self.index) - self.index.index(True, len(self.index)-self.zeiger) - 1
        except ValueError:
            pass
        finally:
            self.index.reverse()
        return self.tabelle[self.zeiger][:]

wobei »previous« wegen fehlendem »rindex« nicht gerade schön aussieht.
Also vielleicht statt einer Liste von Flags nur die ausgewählten Einträge als Index speichern:

Code: Alles auswählen

from collections import deque
    …
    def filter_entries(self, test):
        self.index = deque(idx for idx, entry in enumerate(self.tabelle) if test(entry))

    def fetch_next_row(self):
        self.index.rotate(1)
        return self.tabelle[self.index[0]]

    def fetch_previous_row(self):
        self.index.rotate(-1)
        return self.tabelle[self.index[0]]