return gibt keinen returnwert zurück

synapsenstau · Samstag 14. März 2015, 22:51

Ich habe warscheinlich mit einem Anfängerproblem (welcher ich auch bin)zu kämpfen. Ich lese "Python 3 - Das umfassende Handbuch" (Galiloecomputing) und noch 3 Online Tutorials und bin der Meinung das ich es genau so gemacht habe wie es überall steht, aber die Funktion gibt mir leider keinen Wert zurück.

Code: Alles auswählen

def progressbar(progbar):
    global metadates
    metadateslen = len(metadates)
    for i in range(0, metadateslen):
        if "time" in str(metadates[i]):
            dummy = metadates[i]
            max_time = int(dummy[9:])
    position = int(round(metadates[metadateslen - 2]))
    barindex = round(position / max_time * 10)
    progbar = "["
    for i in range(1, barindex):
        progbar = progbar + "-"
    progbar = progbar + "|"
    for i in (list(range(10, barindex, -1))):
        progbar = progbar + "-"
    progbar = progbar + "]"
    return progbar

balken = ""
progressbar(balken)
print((balken))

var info:
metadates: Liste mit den metadaten vom Vlc PLayer
position: aktuelle Player position in s
max_time: länge des Tracks in s

Die Funktion an sich läuft. Wenn in inerhalb von ihr "print(progbar)" mache gibt sie mir nen Vortschritsbalken aus (so wie gewünscht). Nur wenn ich sie im Main Programm aufrufe kommt nicht (Z.:21).

Wo ist der haken, bzw was mache ich falsch

Üpsilon · Samstag 14. März 2015, 23:32

Code: Alles auswählen

balken = progressbar("")

Deine Progressbar-Funktion ist übrigens ... äh ... naja, man könnte, nein man MUSS sie schöner programmieren:

Man kann direkt über die Liste metadates iterieren, ohne den Umweg über range(len.

Das global ist voll unnötig. Du änderst metadates ja nich. Stattdessen willst du es als Parameter übergeben. Dann siehst der Leser sofort, wovon das Funktionsergebnis abhängt und man kann es isoliert testen.

Das übergeben einer Anfangs-progbar ist hingegen unnötig, weils ja sofort überschrieben wird.

Man kann Strings auch multiplizieren. zB "-" * 5 wertet zu "-----" aus.

Edit: Nutze doch statt der hundert verschiedenen Bücher besser das offizielle: http://py-tutorial-de.readthedocs.org/de/python-3.3/

BlackJack · Samstag 14. März 2015, 23:41

@synapsenstau: Die Funktion gibt etwas zurück, aber Du machst damit dann nichts. Du benutzt da doch einige Funktionen die einen Rückgabewert haben, wie `len()`, `int()`, und `round()` zum Beispiel. Da siehst Du doch wie man mit einem Rückgabewert umgeht. Der Funktionsaufruf wird zu diesem Rückgabewert ausgewertet. Und den kann man dann entweder direkt in einem anderen Ausdruck verwenden in dem man den Aufruf an der entsprechenden Stelle in den Ausdruck schreibt. Oder man weist den Rückgabewert einem Namen zu mit ``=``.

Das Argument `progbar` macht keinen Sinn denn der übergebene Wert wird innerhalb der Funktion nirgends verwendet.

Ebenfalls sinnfrei ist die ``global``-Anweisung. Und die solltest Du am besten ganz schnell wieder vergessen. Des weiteren sollte eine Funktion ausser Konstanten nichts verwenden was nicht als Argument übergeben wurde. Sonst hat man ganz schnell sehr unübersichtlichen und schwer nachvollziehbaren und testbaren Code.

Die erste Schleife in der Funktion ist „unpythonisch” denn statt den Umweg über einen Index `i` zu nehmen, kann man in Python *direkt* über die Elemente von Sequenzen (Listen, Tupel, …) iterieren.

Die erste Schleife ist ausserdem fehleranfällig. Der Teilzeichenkette 'time' könnte auch irgendwo in anderen Metadaten wie Titel oder Kommentar vorkommen und dann fällt der Versuch alles ab dem Index 9 in eine ganze Zahl umzuwandeln *sehr* wahrscheinlich auf die Nase, und für den unwahrscheinlichen Fall das nicht, dann ist es ziemlich sicher nicht die Zeit die man dann hat. Der `str()`-Aufruf macht wohl keinen Sinn. Oder welchen Typ haben die Einträge in `metadates`. (Was übrigens `metadata` heissen müsste, sind ja keine Datumsangaben.)

Das die aktuelle Position immer das vorletzte in `metadates` sein muss halte ich auch für eine gewagte Annahme. Insgesamt ist das parsen vielleicht etwas fragil. Wenn man die Daten doch nur direkt als Datenstruktur abfragen könnte.

Die beiden anderen Schleifen kann man sich sparen wenn man die Multiplikation von Zeichenketten mit ganzen Zahlen verwendet:

Code: Alles auswählen

In [4]: '*' * 10
Out[4]: '**********'

In [5]: len('*' * 10)
Out[5]: 10

Dann braucht man die Zeichenkette auch nicht so ineffizient über wiederholtes ``+`` aufbauen sondern kann Zeichenkettenformatierung per `format()`-Methode auf Zeichenketten verwenden.

synapsenstau · Sonntag 15. März 2015, 00:10

Üpsilon hat geschrieben:
Code: Alles auswählen
balken = progressbar("")

Danke das war der Fehler. Jetzt gehts.....

Deine Progressbar-Funktion ist übrigens ... äh ... naja, man könnte, nein man MUSS sie schöner programmieren:
Man kann direkt über die Liste metadates iterieren, ohne den Umweg über range(len.

Schöner geht immer. Das mit über Listen itieren werde ich mir morgen noch mal anschauen. Anfängerbonus...

Das global ist voll unnötig. Du änderst metadates ja nich. Stattdessen willst du es als Parameter übergeben. Dann siehst der Leser sofort, wovon das Funktionsergebnis abhängt und man kann es isoliert testen.

Jep die globale ist weg. Nachdem das übergenben der Var geklappt hat brauchte ich sie nicht mehr. War ne Krücke, damit ich in allen Funktionen sie zur Verfügung habe.

Man kann Strings auch multiplizieren. zB "-" * 5 wertet zu "-----" aus.

Ja, da war was. Schaue ich mir auch morgen noch mal an.

Edit: Nutze doch statt der hundert verschiedenen Bücher besser das offizielle: http://py-tutorial-de.readthedocs.org/de/python-3.3/

Ja das ist auch gerade immer offen in einem Tab, wie noch ein paar andere Tut's.

Danke für die Hinweise und werde ihnen morgen nachgehen.

Üpsilon · Sonntag 15. März 2015, 00:19

Ich hab grade zwanzig Minuten gebraucht, um diesen eigentlich voll simplen Code zu verstehen. Und da kann ich jez noch mehr Senf dazu geben.

Zeile 1: Nimm metadates als Argument.

Zeilen 2 und 3 fliegen erstmal raus.

Zeilen 4-7 hab ich jetzt so verstanden: Du willst das letzte Element aus der Liste metadates, in dem "time" drin vorkommt, und davon alles ab der neunten Stelle? Das würde ich so machen (UngeTestet):

Code: Alles auswählen

for date in reversed(metadates):
    if "time" in str(date):
        max_time = date[9:]
        break

Hier gehe ich rückwärts durch metadates durch, schaue nach "time", wenn ja hol ich mir die max_time und dann spring ich sofort raus.
Wenn du magst, kannste das in eine eigene Funktion auslagern - dann halt return statt max_time= und das break braucht man dann auch nicht mehr.

Zeile 8: liste[len(liste)-2] geht übrigens auch mit liste [-2]. Das int ist auch unnötig, weil du ja roundest und danach weiterrechnest, also isses wurscht, obs int oder float ist.

Zeile 9: Du benutzt Python3, gell? Da kann man mit // Ganzzahldivision machen: 7//3 wertet aus zu 2. Dann round weg.

Mit Zeile 11-12 machst du barindex-1 Striche. Also mach "-" * (barindex-1). Ähnlich bei Zeile 14-15.

Das alles kann man so zusammensetzen:

Code: Alles auswählen

return "[{}|{}]".format(striche_links, striche_rechts)

Sieht kryptisch aus, isses aber nicht. Es werden einfach nur die {} ersetzt, durch das, was man hinten mitgibt.

So kann man das ganze in 10 Zeilen formulieren.

Liebe Grüße, Y.

Üpsilon · Sonntag 15. März 2015, 00:49

Das mit dem über Listen iterieren werde ich mir morgen nochmal anschauen

Mach das. In vielen Programmiersprachen gehen for-Schleifen idR mit Zahlen, deswegen machen viele Leute das auch in Python so. Dabei sind die tollen Listen neben Blockbegrenzung durch Einrückung und der allgemein höheren Göttlichkeitsstufe eines der herausragenden Features von Python.

@alle Weil ich nachts immer so tolle Ideen habe:

Code: Alles auswählen

max_time = [date[9:] for date in metadates if "time" in str(date)][-1]

Wer schafft es kürzer?

BlackJack · Sonntag 15. März 2015, 01:50

@Üpsilon: Ich weiss nicht ob es Sinn macht fehlerhaften Code kürzer zu machen. Das `str()` ist überflüssig und ansonsten gilt was ich schon über die Robustheit, bzw. das Fehlen davon, bereits gesagt habe. Das Konstrukt garantiert in keiner der bisher gezeigten Formen das man dadurch die Länge der Mediendatei aus den Daten herausgefischt bekommt.

Das Attribut ('time:vlc') was dafür abgefragt wird halte ich auch für ungeeignet denn die Spezifikation von VLC für die D-Bus-Schnittstelle die ich finden konnte ist veraltet und sagt gleich am Anfang selbst das man auf nichts bauen sollte was da steht: https://wiki.videolan.org/DBus-spec/

Vielversprechender ist da die MPRIS Spezifikation die man bei Freedesktop.org findet: http://specifications.freedesktop.org/m ... ec/latest/

Die MPRIS-Spezifikation dokumentiert zum einen 'mpris:length' und zum anderen stimmt dort dann auch gleich die Einheit (Mikrosekunden) mit der des `Position`-Property überein, so dass man da nichts umrechnen muss wenn man den prozentualen Anteil berechnen möchte.

Was an der MPRIS-API unschön ist: Man kann nicht atomar Länge *und* Position abfragen, so dass es passieren kann das die abgefragte Länge zu einem anderen Track gehört als die abgefragte Position. Man sollte da also unbedingt eine Plausibiltätsprüfung einbauen und muss mit kleinen Schönheitsfehlern rund um den Trackwechsel rechnen. Sich per Signal über die Änderungen informieren zu lassen, in der Hoffnung dass dann Metadaten und Position immer zusammenpassen, weil man ja beim Wechsel des Tracks informiert wird und dann weiss das eine bereits vorher übermittelte Position ungültig ist, geht nicht weil die Positionsänderung keine Signale auslöst. Würde bei der Mikrosekundenauflösung den Bus wohl auch ziemlich dicht machen.

Oh, und es gibt natürlich Tracks bei denen keine Länge ermittelt werden kann (Live-Streams z.B.), dass heisst man sollte auch damit klar kommen können das in den Metadaten keine Länge enthalten ist!

synapsenstau · Sonntag 15. März 2015, 12:02

Zwischenbericht:

BlackJack hat geschrieben: Die erste Schleife in der Funktion ist „unpythonisch” denn statt den Umweg über einen Index `i` zu nehmen, kann man in Python *direkt* über die Elemente von Sequenzen (Listen, Tupel, …) iterieren.

So besser, s.u.?

Die erste Schleife ist ausserdem fehleranfällig. Der Teilzeichenkette 'time' könnte auch irgendwo in anderen Metadaten wie Titel oder Kommentar vorkommen und dann fällt der Versuch alles ab dem Index 9 in eine ganze Zahl umzuwandeln *sehr* wahrscheinlich auf die Nase, und für den unwahrscheinlichen Fall das nicht, dann ist es ziemlich sicher nicht die Zeit die man dann hat.

Also, das hier ist der Inhalt der aktuellen metadate:

Code: Alles auswählen

['vlc:length: 480242', 'vlc:publisher: 9', 'vlc:publisher: Woorpz', 'vlc:time: 480', 'xesam:album: Alien Friendship (Compiled by Fido)', 'xesam:artist: Kinetic', 'xesam:contentCreated: 2015', 'xesam:genre: Psychedelic', 'xesam:title: Hana (Original Mix)', 'xesam:tracknumber: 1', 'xesam:url: file:///media/dev/fs2/tmp/complete_down/VA-Alien_Friendship_%28Compiled_By_Fido%29-WEB-2015-JUSTiFY/01-kinetic-hana_%28original_mix%29.mp3', 126.387828, 'VLC media player']

Bei der D-bus abfrage der Metadaten vom Vlc Player ist das erste "time" was kommt immer "vlc:time: xxx". Damit mir keine weiteres "time" in Titel o.ä. verlasse ich jetzt vorzeitig die for-Schleife mit "break".

Das die aktuelle Position immer das vorletzte in `metadates` sein muss halte ich auch für eine gewagte Annahme. Insgesamt ist das parsen vielleicht etwas fragil. Wenn man die Daten doch nur direkt als Datenstruktur abfragen könnte.

Die aktuelle Position ist IMMER die vorletzte Position. In der vorhergehenden Funktion habe ich sie an die "metadate" Liste angehängt ("metadate.append(playtime_s)", genau wie Player Identify).

Hier jetzt mal die aktuelle Code. Werde mich jetzt an die "progbar" machen....

Code: Alles auswählen

def progressbar(progbar, metadate):
    for i in metadate:
        if "time" in i:
            max_time = int(i[9:])
            break
    position = int(round(metadate[len(metadate) - 2]))
    barindex = round(position / max_time * 10)

Zur allg. Info. Diese Projekt bassiert auf einem Shell Script, was bei den Examples bei Eiskaltdcpp beiliegt und im Mainchat den aktuell gehörten Track anzeigt. Mir hat es selber nicht so ganz gefallen. Fürs erste wollte ich es erst mal in Python portieren (Übung, Syntax verinnerlichen und so) und dann anpassen. Und das ich Übung nötig habe sieht mal ja,

Hier mal das "orginal" Script: http://pastebin.com/4G74k7g6

Sirius3 · Sonntag 15. März 2015, 12:10

@synapsenstau: Warum prüfst Du ob in dem String 'time' vorkommt, obwohl doch gesichert ist, dass der gesuchte String mit 'vlc:time: ' anfängt? i ist kein guter Variablenname, da jeder gleich an einen Laufindex denkt und nicht an einen String.

synapsenstau · Sonntag 15. März 2015, 13:02

@Sirius3:
Ja du hast ja recht. Sicher ist sicher: "time" in "vlc:time:" , 'i' in 'date' geändert.

Üpsilon hat geschrieben:
Zeile 9: Du benutzt Python3, gell? Da kann man mit // Ganzzahldivision machen: 7//3 wertet aus zu 2. Dann round weg.

Danke für den hinweis. Habe ich gleich an anderer stelle benutzt.

Mit Zeile 11-12 machst du barindex-1 Striche. Also mach "-" * (barindex-1). Ähnlich bei Zeile 14-15.
Das alles kann man so zusammensetzen:
Code: Alles auswählen
return "[{}|{}]".format(striche_links, striche_rechts)
Sieht kryptisch aus, isses aber nicht. Es werden einfach nur die {} ersetzt, durch das, was man hinten mitgibt.
So kann man das ganze in 10 Zeilen formulieren.

Und nochmals danke. Auch geändert und nu sind es nur noch 10 Zeilen.

Code: Alles auswählen

def progressbar(progbar, metadate):
    for date in metadate:
        if "vlc:time:" in date:
            max_time = int(date[9:])
            break
    position = metadate[-2]
    barindex = round(position / max_time * 10)
    dash_left = "-" * (barindex - 1)
    dash_right = "-" * (10 - barindex)
    return "[{}|{}]".format(dash_left, dash_right)

Edit:

BlackJack hat geschrieben: Oh, und es gibt natürlich Tracks bei denen keine Länge ermittelt werden kann (Live-Streams z.B.), dass heisst man sollte auch damit klar kommen können das in den Metadaten keine Länge enthalten ist!

Danke für den hinweis. Habe mir gerade mal angeschaut was sich dann ändert und es besteht handlungsbedarf.....

BlackJack · Sonntag 15. März 2015, 13:23

@synapsenstau: Wer garantiert Dir denn die Reihenfolge und die Attribute von den Metadaten?

Das in der Liste kein 'mpris:length' vorkommt ist komisch, denn das gehört ja eigentlich zur Spezifikation von dem D-Bus-Interface‽ Wobei das ja noch optional ist, aber eine 'mpris:trackid' *muss* laut Spezifikation immer vorhanden sein.

Das Werte an bestimmten ”festen” Indexen in einer Liste besondere Bedeutungen haben ist unschön. Das führt zu ”magischen” Indexwerten von denen man die Bedeutung kennen muss und ist schlecht lesbar. Hier wird das noch mal verschlimmert dadurch das der feste Index relativ zum Ende einer Liste mit variabler Länge angegeben wird. Übersichtlich und verständlich sieht anders aus.

Wenn man das sauberer lösen wollte, dann würde man nicht unterschiedliche Informationen in eine Liste stecken sondern zum Beispiel die Metadaten in einem Wörterbuch ablegen. Dann kann man statt 'time' irgendwo in einer Zeichenkette zu suchen gezielt über den Schlüssel 'vlc:time' auf den Wert zugreifen.

Funktionen werden üblicherweise nach Tätigkeiten benannt weil die etwas tun. Dann kommt man auch nicht so schnell in die Verlegenheit komische Abkürzungen wie `progbar` verwenden zu müssen weil der Name für einen passiven Wert schon für eine Funktion verwendet wird. Wobei das Argument ja überhaupt nicht verwendet wird.

synapsenstau · Sonntag 15. März 2015, 13:51

BlackJack hat geschrieben: Das in der Liste kein 'mpris:length' vorkommt ist komisch, denn das gehört ja eigentlich zur Spezifikation von dem D-Bus-Interface‽ Wobei das ja noch optional ist, aber eine 'mpris:trackid' *muss* laut Spezifikation immer vorhanden sein.

Konnten sie auch nicht. Ich hatte liste 'metadata' beschnitten nach der Abfrage 'del metadata[0:4]', weil ich diese daten für überflüssig/nicht benötigt befunden hatte. Habe das 'del...' gerade wieder entfernt, weil diese Holzhammermethode nicht bei Internetstreams funktioniert.

Das Werte an bestimmten ”festen” Indexen in einer Liste besondere Bedeutungen haben ist unschön. Das führt zu ”magischen” Indexwerten von denen man die Bedeutung kennen muss und ist schlecht lesbar. Hier wird das noch mal verschlimmert dadurch das der feste Index relativ zum Ende einer Liste mit variabler Länge angegeben wird. Übersichtlich und verständlich sieht anders aus.
Wenn man das sauberer lösen wollte, dann würde man nicht unterschiedliche Informationen in eine Liste stecken sondern zum Beispiel die Metadaten in einem Wörterbuch ablegen. Dann kann man statt 'time' irgendwo in einer Zeichenkette zu suchen gezielt über den Schlüssel 'vlc:time' auf den Wert zugreifen.

Jep, das ist was drann. Überlege mir ne Lösung um es schöner zu machen......

Funktionen werden üblicherweise nach Tätigkeiten benannt weil die etwas tun. Dann kommt man auch nicht so schnell in die Verlegenheit komische Abkürzungen wie `progbar` verwenden zu müssen weil der Name für einen passiven Wert schon für eine Funktion verwendet wird. Wobei das Argument ja überhaupt nicht verwendet wird.

Richtig, wird geändert....