Umständlich? Völlig falsch? Code zum zerreissen ;o)

Wenn du dir nicht sicher bist, in welchem der anderen Foren du die Frage stellen sollst, dann bist du hier im Forum für allgemeine Fragen sicher richtig.
Antworten
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

So, ich habe mal meinen Code ein wenig anonymisiert (weil gewisse Daten nicht frei im Netz verfügbar sein sollen).

Es ist ein Player, bzw. es wird damit der mplayer benutzt, um einen Stream zu hören Zugangsdaten werden eingebunden (ist ein geschützter Stream), Lied-Tags ausgelesen und angezeigt, und aus einer SLT-Datei im Netz die Infos über den Stream selbst ausgelesen.

Wäre nett, wenn mal der ein oder andere drüber sehen könnte, und mir sagen könnte, was daran absoluter Müll ist, oder was man vielleicht wesentlich einfacher und effizienter gestalten könnte.
Ich bin mir nahezu sicher, dass das einiges sein wird, aber ich möchte mich gerne bessern. :o) Und ich bin mir sicher, dass das an einigen Stellen möglich ist.

http://paste.pocoo.org/show/129309/

Das dazugehörige XRC:
http://paste.pocoo.org/show/129311/
Dauerbaustelle
User
Beiträge: 996
Registriert: Mittwoch 9. Januar 2008, 13:48

- PEP8 folgen
- global rausnehmen
- Niemals *alle* Fehler mit `except`abfangen
- `if mountpoint == 1` geht auch als `if mountpoint` (ebenso `== True`)
- entweder alles Deutsch oder alles Englisch, Mix draus ist doof
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

Werd's dann nochmal in der Richtung überarbeiten

Wegen den globals allerdings... ich weiß nicht, wie ich die umgehen soll. Hab ich schon hin und her überlegt, aber beispielsweise "mplayer_beendet" kann aus unterschiedlichen Gründen geändert werden, ebenso "proc"... müsste ich dann schon im Bindung für "stop" definieiert haben, aber ich kann es ja erst an einer anderen stelle zusammen setzen.

Vielleicht denke ich in der Richtung auch falsch, aber da wäre ich für Hilfe dankbar.
Dauerbaustelle
User
Beiträge: 996
Registriert: Mittwoch 9. Januar 2008, 13:48

Du hast doch die Anwendung eh schon in einer Klasse, warum machst du `mplayer_beendet` dann nicht einfach als Klassen- oder Instanzvariable?
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

Hm, gerade wieder was dazu gelernt. danke. :o)


BTW: Kann jemand sagen/sehen, ob am XRC irgendwas falsch ist? Weil, wenn ich die im XRCed bearbeiten und wieder speichern will, kommt grundsätzlich eine Fehlermeldung.
fhoech
User
Beiträge: 143
Registriert: Montag 9. April 2007, 18:26

Die XRC ist in Ordnung, XRCEd stört sich nur an den Umlauten (ist wohl ein Bug in XRCed).
Aber: Ich würde Dir auf jeden Fall raten, die Elemente sauber mit Sizern und nicht absolut zu positionieren.
Einen Font würde ich auch nicht unbedingt vorgeben, damit die jeweiligen OS- bzw. Theme-Defaults benutzt werden.
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

Danke.


Ich gebe zu, mit den Sizern komme ich absolut nicht klar. Bei mir gehen die immer eigene Wege, und selbst als ich es das erste Mal mit gemacht habe (mit wxGlade) habe ich keinen Vorteil darin gesehen, da es bei Veränderung der Fenstergröße irgendwie dermaßen doof aussah...
fhoech
User
Beiträge: 143
Registriert: Montag 9. April 2007, 18:26

Mir ist noch aufgefallen, dass Du in Deinem Code keine Unicode-Strings verwendest (wobei, das stimmt nur für Python 2.x, nicht für 3.x - ich nehme jetzt aber mal an, dass Du mit ersterem unterwegs bist). Zumindest bei den Strings, die Umlaute o.ä. enthalten könn(t)en, solltest Du Unicode nehmen, sonst gibts Darstellungsprobleme, wenn ein Nutzer nicht UTF-8 als System-Encoding eingestellt hat.
Ich gebe zu, mit den Sizern komme ich absolut nicht klar. Bei mir gehen die immer eigene Wege, und selbst als ich es das erste Mal mit gemacht habe (mit wxGlade) habe ich keinen Vorteil darin gesehen, da es bei Veränderung der Fenstergröße irgendwie dermaßen doof aussah...
Ja, das Layout mit Sizern ist erst mal komplexer als ohne. Und um die Vorteile zu sehen, muss man sich erst einmal die Nachteile eines Layouts ohne Sizer verdeutlichen, z.B. wenn ein Nutzer den dpi-Wert seines OS verändert. Bilder sprechen mehr als 1000 Worte, darum hier mal ein Beispiel, bei dem die dpi auf 144 gesetzt wurden (ja, ist bei normalen Monitoren ein übertriebener Wert, aber dadurch wirds deutlicher):

Absolut positioniertes Layout bei 144dpi
Mit Sizern positioniertes Layout bei 144dpi

Hier noch das von mir mit Sizern umgebaute XRC, bei denen für kein einziges Element irgendeine Größe vorgegeben wurde. Alles wird anhand des Platzes, den die Elemente beanspruchen, dynamisch angepasst. Außerdem gibt es keine Encoding-Vorgaben mehr für die Fonts, da das zu Warnmeldungen führen kann, wenn kein Font mit entsprechendem Encoding gefunden wird.

http://paste.pocoo.org/show/130138/

Das XRC enthält jetzt auch die Statusleiste, die Du vorher im Code hattest. Darum musst Du Deinen Code in Zeile 44 wie folgt ändern:

Code: Alles auswählen

        self.frmStatusbar = xrc.XRCCTRL(self.frame, "frmStatusbar")
        self.frame.Fit()
(das Fit() sorgt dafür, dass die Statusleiste nicht den Text überlappt)

(Edit: XRC überarbeitet, das Layout passt jetzt bei Änderung der Fenstergröße sinnvoll (imho) in der Breite an.)
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

Ups, das mit den unicode-Strings hab ich echt vergessen. *grmpf*
Unter Linux fällt es nicht auf, daher vermutlich. Danke für den Hinweis.
Habe ich jetzt soweit eingebaut, allerdings ist dabei beim Test unter Win auch ein merkwürdiges Phänomen aufgetreten, welches ich nicht festmachen kann, wieso das ist.
Und zwar ist die Anzeige beim ersten Titel noch richtig, beim Wechsel auf den nächsten wird aber ein Teil der Anzeige nicht dargestellt, genauer im Titel.
Aber nicht dass Buchstaben fehlen, man kann noch Reste vom vorherigen sehen. Mehr als wenn da etwas drüber geschoben wird.
Ich habe es unter anderem so versucht:

Code: Alles auswählen

    
def artistanzeige(self, artist):
    # Anzeige des Interpreten
    if artist != self.lblInterpret.Label:
        self.lblInterpret.Label = unicode(artist, 'utf-8')
und

Code: Alles auswählen

def artistanzeige(self, artist):
    # Anzeige des Interpreten
    if artist != self.lblInterpret.Label:
        self.lblInterpret.Label = artist.decode("utf-8")
Beides liefert das gleiche Ergebnis, bzw. die gleiche Fehleranzeige.
Zufällig eine Idee was das verursacht?

Sizer: Verstehen tu ich den Nutzen schon, auch wenn beim ir der Fall noch nicht aufgetreten ist. ;o) Aber danke erstmal für das geänderte XRC, ich werde mir das mal zu Gemüte führen, vielleicht macht es das so für mich verständlicher wegen dem wie und wo.

Das komische Aussehen war übrigens das, dass die Breite zwar angepasst wurde, aber die Höhe nicht. Daher sah es dann in meinen Augen merkwürdig aus. Also so ähnlich, wie es jetzt auch ist.
Das hatte ich nie hin bekommen, dass er auch die Höhe anpsst, dann hatte ich es gelassen.

Wegen der Statusbar... ist übrigens seltsam... genauso hatte ich das - könnte ich wetten - vorher auch drin, aber ich konnte es nicht ansprechen, egal was ich gemacht habe. Daher hatte ich es per Code eingebunden. Nun funktioniert es... Wer weiß, was ich da falsch hatte...
EyDu
User
Beiträge: 4881
Registriert: Donnerstag 20. Juli 2006, 23:06
Wohnort: Berlin

Ich hab noch einen kurzen Blick drüber geworfen und ein paar weitere Anmerkungen:

- MyApp ist ein reichlich nichtssagender Name
- Zeilen 75-79:

Code: Alles auswählen

mountpoint = self.radMountpoint.GetSelection()
url = ("@xxx.xxx.xxx/xxx.ogg", "@sss.sss.sss/xxxXXX.ogg")[mountpoint]
- Such in der Doku mal nach "String Formatting", ist z.B. ist Zeile 92 sinnvoll
- ein "except" sollte niemals angewendet werden, ohne dass man explizit angibt, was abgefangen soll. Andernfalls gehen wirklich alle Meldungen, auch Fehlermeldungen, verloren.
- Zeile 130: such mal nach der "enumerate"-Funktion
- die sender-Methode würde ich anders schreiben (beachte auch die neue Bedingung des if):

Code: Alles auswählen

def sender(self, info):
    if info[1]:
        streamtitel = info[1] + " mit " + '"' + info[5] + '"'
    else:
        streamtitel = "Keine Sendung oder Moderator nicht richtig eingerichtet"
self.lblStreamtitel.Label = streamtitel
Man könnte hier auch noch zusätzlich meinen zweiten Tipp anwenden.
- Methoden und Funktionen werden als Verben oder nach Tätigkeiten benannt, du verwendest aber öfters Nomen (sender, artistanzeige, ...).
- "read-Methode": hier solltest du wirklich mal über Konstanten nachdenken. Dann wird es viel flexibler:

Code: Alles auswählen

STATUS_ARTIST = "Artist:"
if STATUS_ARTIST in line:
    artist = line[len(STATUS_ARTIST)+2:]
    wx.CallAfter(self.artistanzeige, artist)
- es bietet sich auch die "startswith"-Methode auf den Strings an, da du dies eigentlich ausdrücken willst. "in" matcht bei beliebigen Positionen.
- "elif" kann bei der Gelegenheit auch verwendet werden, da sich einige Fälle offensichtlich ausschließen.
- "updatestatus" macht den Eindruck, als wolle es noch zusammengefasst werden.
- Dateien sollten mit "open" geöffnet werden, nicht mit "file". Da fällt mir auf, dass auch der ein oder andere mögliche Fehler abgefangen werden sollte.
- die Schleife in "laden" kann noch vereinfacht werden:

Code: Alles auswählen

while True:
    line = f.readline().strip("\n")
    if not line:
        break
    elif i == 1:
        benutzername = line[::-1]
    elif i == 2:
        passwort = line[::-1]
    elif i == 3:
        mountpoint = int(line)
    i += 1
Das Leben ist wie ein Tennisball.
fhoech
User
Beiträge: 143
Registriert: Montag 9. April 2007, 18:26

Und zwar ist die Anzeige beim ersten Titel noch richtig, beim Wechsel auf den nächsten wird aber ein Teil der Anzeige nicht dargestellt, genauer im Titel.
Aber nicht dass Buchstaben fehlen, man kann noch Reste vom vorherigen sehen. Mehr als wenn da etwas drüber geschoben wird.
Seltsam. Ich kanns leider nicht testen, zuerst dachte ich, da fehlt evtl. ein wx.CallAfter beim Setzen des Labels aus dem andren Thread heraus, aber das benutzt Du ja schon.
Versuch mal die 'Fit()'-, 'Refresh()'- oder 'Update()'-Methoden am Label (oder vielleicht dem Frame), nachdem er gesetzt wurde, oder mit 'Freeze()' einzufrieren, bevor Du den Label setzt, und dann 'Thaw()'. Sorry, ich rate hier gerade nur, keine Ahnung, obs was bringt.
Aber danke erstmal für das geänderte XRC, ich werde mir das mal zu Gemüte führen, vielleicht macht es das so für mich verständlicher wegen dem wie und wo.
Wenn Fragen sind, nur zu. Ich bin zwar kein wx-Crack, aber mit Sizern kenn ich mich zwischenzeitlich recht gut aus (solangs nicht um BagSizer geht) ;)
Das komische Aussehen war übrigens das, dass die Breite zwar angepasst wurde, aber die Höhe nicht. Daher sah es dann in meinen Augen merkwürdig aus. Also so ähnlich, wie es jetzt auch ist.
Ah so. Eine Höhenanpassung wäre ansich kein Problem, ich hab sie aber aus gutem (imho) Grund weggelassen: Sie bringt für den Nutzer keinen Gewinn (ausser mehr Freiraum um die Controls, oder grössere Controls - wobei für meinen Geschmack sieht beides auch merkwürdig aus, da die Schriftgröße ja gleich bleibt). Bei der Breitenanpassung kann man wenigstens dann auch mehr Text sehen (wenn er lang genug ist), darum machts da imho auch Sinn.
TiKaey
User
Beiträge: 84
Registriert: Montag 24. November 2008, 20:48

Oh, 'ne Menge input. :o)
Dann habe ich ja die Tage was zu tun. :o)

Nochmal bzgl. des Anzeigefehlers,.... das seltsame ist halt, dass dieser Fehler nur auftritt, wenn ich versuche ihm unicode beizubringen. Ansonsten nicht. Das ist das was ich extrem seltsam finde, vor allem da ja nicht einfach Buchstaben fehlen, sondern wie mit Radiergummi behandelt aussehen.

Ansonsten hab ich jetzt ja erstmal was zu tun. Danke. :o)
Antworten