Hallo liebe Community!
Ich arbeite gerade das Buch Objektorientierte Programmierung mit Python 3 durch. Nun hab ich mein erstes "sinnvolles" Skript geschrieben - einen Minecraft Installer in Python3
Da ich aber noch ein ziemlicher Anfänger bin wollte ich euch Fragen, was ich in diesem Skript noch verbessern könnte?
Klick mich!
Danke für eure Verbesserungsvorschläge
P.S.: Ich wusste nicht genau in welches Forum (Showcase oder Codesnippet?) ich so einen "Verbesserungsvorschlag" posten soll. Falls es im falschen Forum ist, könnte es ein Moderator in das richtige Forum verschieben?
Minecraft Installer
- Hyperion
- Moderator
- Beiträge: 7478
- Registriert: Freitag 4. August 2006, 14:56
- Wohnort: Hamburg
- Kontaktdaten:
Hallo,
bei diesen beiden Foren sind wir uns hier selten sicher, was wo wirklich reinpasst
Zu Deinem Code kann man so einiges anmerken:
- Wozu diese Klassen? In Wirklichkeit "missbrauchst" Du diese als Modul. Sie haben keinen "inneren" Zustand und kapseln auch nichts. Das könnte man alles eleganter ohne Klassen lösen!
- Es gibt Multiline-Strings (für Zeile 11):
Wenn man dabei eine Einrückung zur besseren Lesbarkeit bewahren will, kann man mittels `dedent`-Modul die Einrückungstiefe eleminieren.
- Strings sollte man nicht per `+`-Operator zusammenbauen - auch wenn das natürlich geht. Benutze dafür z.B. die `"".format()`-Methode:
- Zeile 37: Öffnet man Dateien mit `with`, braucht man diese nicht explizit zu schließen. Das `f.close` kannst (und solltest!) Du Dir sparen.
- Ich weiß nicht genau, ob es eine elegante Möglichkeit gibt, einzelne Abschnitte in einem Python-Programm mit speziellen Rechten auszuführen, aber Deine Idee, `sudo` mit `os.system` zu koppeln klingt für mich wenig sinnvoll. Zum einen ist das sehr plattformspezifisch (Unter Windows wird das alles nicht klappen!), zum anderen wird es schwierig, eine sinnvolle Fehlerbehandlung durchzuführen. Bei Dir fehlt die ja auch komplett. Wenn Du die entsprechenden Funktionen aus den `os`- oder dem `shutil`-Modulen benutzt, wird das schon einfacher zu realisieren. Insofern würde ich dann ggf. für die komplette Ausführung des Programms Administratorrechte "verlangen", aber dafür die Funktionen aus der Standard-Lib benutzen.
- Anstelle von `os.system` solltest Du auch eher auf das `subprocess`-Modul setzen.
- Die Methode `troubleshoot` ist viel zu "komplex". Du erledigst dort zig verschiedene Dinge, die man auf den ersten Blick wunderbar in separate Funktionen auslagern könnte. Zudem erwarte ich doch, dass das "Dispatching" (also das Verzweigen, je nach Auswahl) außerhalb erledigt wird. Du hast doch extra eine `UserInterface`-Klasse implementiert; wieso wird das dann nicht auch darin erledigt? (Obwohl ich immer noch dafür plädiere, diese Klassen einzustampfen!)
- Dir fehlt es an einer Struktur für Deine Menüeinträge. Was passiert, wenn Du das Menü an zwei Stellen ändern willst und noch Einträge dazu kommen, die dann auch noch an verschiedenen Stellen eingefügt werden sollen? So etwas kann man einfacher lösen, indem man für ein Menü eine passende Datenstruktur benutzt und die Verzweigung dann "berechnet", anstatt sie hart in langen `if...elif`-Kaskaden zu kodieren. (Für diese Sache brauchen wir mal eine wiki-Seite )
Beispiel:
Ich habe hier ein Tupel für jeden Menüeintrag benutzt. Als erster Eintrag steht ein String, der den Eintrag beschreibt, als zweites Element füge ich ein Funktionsobjekt hinzu, welches bei Auswahl aufgerufen werden soll. Das ist die "Magie", die für viele Anfänger neu / unbekannt ist. In Python ist alles ein Objekt und deswegen kann ich alles auch an Stellen verwenden, an denen ein Objekt stehen darf. Also auch in Listen, Dictionaries, Tupeln usw.
Meine Struktur kann ich nun einfach durchgehen und die Ausgabe erzeugen. Die Benutzereingabe habe ich nur mal angedeutet, aber da hast Du ja etwas brauchbares. Bei positiver Verifikation benutzt Du den Index (-1, da Indizies bei 0 anfangen, unsere Ausgabe aber bei 1 anfängt) und pickst den zweiten Eintrag heraus, also die aufzurufende Funktion. Diese rufst Du dann einfach auf `()`. Schon landest Du in den Funktionen, die dann eine bestimmte "Arbeit" durchführen.
Willst Du nun Einträge umsortieren, neue Einträge einfügen o.ä., brauchst Du nur die Menüdefinition anpassen. Ich hoffe Du siehst, dass das viel einfacher ist, als Deine Lösung.
Ok,. Du musst noch ein Submenü einbauen... da lasse ich Dich mal selber überlegen, wie man das realisieren könnte
- Zeile 131: Das einzelne `raise` soll genau was bringen?
- Zeile 132: Fange niemals *alle* Ausnahmen ab. Fange nur diejenigen ab, die Du wirklich behandeln willst. In dem Falle wohl einen `ValueError`.
- Zeilen 140f.:
Was soll das bewirken? Wozu der Name `troubleshoot`? Du kannst doch genauso gut gleich
schreiben! Du verlässt die Schleife doch eh erst durch ein `break` und nicht etwa dadurch, dass Du `troubleshoot` auf `False` setzt. Zudem bindest Du im Schleifenrumpf ständig neue Werte an `troubleshoot` - ist das wirklich beabsichtigt? Wäre ein Menüeintrag "0", so würdest Du unfreiwillig die Schleife verlassen
- Als letztes: Es fehlt ein wenig Doku und ggf. ein Lizenzhinweis. Zumindest einen Modul-Docstring mit Hinweis auf die Funktionsweise des Moduls könnte man erwarten.
So, das war ziemlich viel... ich hoffe Du kannst da einiges von gebrauchen.
Positiv finde ich übrigens, dass Du Dein Script in einem Pastebin ausgelagert hast
bei diesen beiden Foren sind wir uns hier selten sicher, was wo wirklich reinpasst
Zu Deinem Code kann man so einiges anmerken:
- Wozu diese Klassen? In Wirklichkeit "missbrauchst" Du diese als Modul. Sie haben keinen "inneren" Zustand und kapseln auch nichts. Das könnte man alles eleganter ohne Klassen lösen!
- Es gibt Multiline-Strings (für Zeile 11):
Code: Alles auswählen
s = """
Hallo
Welt!
"""
- Strings sollte man nicht per `+`-Operator zusammenbauen - auch wenn das natürlich geht. Benutze dafür z.B. die `"".format()`-Methode:
Code: Alles auswählen
s = "Hallo, {}".format("Hyperion")
- Ich weiß nicht genau, ob es eine elegante Möglichkeit gibt, einzelne Abschnitte in einem Python-Programm mit speziellen Rechten auszuführen, aber Deine Idee, `sudo` mit `os.system` zu koppeln klingt für mich wenig sinnvoll. Zum einen ist das sehr plattformspezifisch (Unter Windows wird das alles nicht klappen!), zum anderen wird es schwierig, eine sinnvolle Fehlerbehandlung durchzuführen. Bei Dir fehlt die ja auch komplett. Wenn Du die entsprechenden Funktionen aus den `os`- oder dem `shutil`-Modulen benutzt, wird das schon einfacher zu realisieren. Insofern würde ich dann ggf. für die komplette Ausführung des Programms Administratorrechte "verlangen", aber dafür die Funktionen aus der Standard-Lib benutzen.
- Anstelle von `os.system` solltest Du auch eher auf das `subprocess`-Modul setzen.
- Die Methode `troubleshoot` ist viel zu "komplex". Du erledigst dort zig verschiedene Dinge, die man auf den ersten Blick wunderbar in separate Funktionen auslagern könnte. Zudem erwarte ich doch, dass das "Dispatching" (also das Verzweigen, je nach Auswahl) außerhalb erledigt wird. Du hast doch extra eine `UserInterface`-Klasse implementiert; wieso wird das dann nicht auch darin erledigt? (Obwohl ich immer noch dafür plädiere, diese Klassen einzustampfen!)
- Dir fehlt es an einer Struktur für Deine Menüeinträge. Was passiert, wenn Du das Menü an zwei Stellen ändern willst und noch Einträge dazu kommen, die dann auch noch an verschiedenen Stellen eingefügt werden sollen? So etwas kann man einfacher lösen, indem man für ein Menü eine passende Datenstruktur benutzt und die Verzweigung dann "berechnet", anstatt sie hart in langen `if...elif`-Kaskaden zu kodieren. (Für diese Sache brauchen wir mal eine wiki-Seite )
Beispiel:
Code: Alles auswählen
main_menu = (
("Install Minecraft", install),
("Uninstall Minecraft", uninstall),
("Troubleshooting", troubleshoot),
("Exit", lambda: sys.exit(0)),
)
for index, item in enumerate(main_menu, 1):
print "{} {}".format(index, item[0])
choice = input(...)
# here is the 'magic'; we call a function, without using its name.
main_menu[choice-1][1]()
Meine Struktur kann ich nun einfach durchgehen und die Ausgabe erzeugen. Die Benutzereingabe habe ich nur mal angedeutet, aber da hast Du ja etwas brauchbares. Bei positiver Verifikation benutzt Du den Index (-1, da Indizies bei 0 anfangen, unsere Ausgabe aber bei 1 anfängt) und pickst den zweiten Eintrag heraus, also die aufzurufende Funktion. Diese rufst Du dann einfach auf `()`. Schon landest Du in den Funktionen, die dann eine bestimmte "Arbeit" durchführen.
Willst Du nun Einträge umsortieren, neue Einträge einfügen o.ä., brauchst Du nur die Menüdefinition anpassen. Ich hoffe Du siehst, dass das viel einfacher ist, als Deine Lösung.
Ok,. Du musst noch ein Submenü einbauen... da lasse ich Dich mal selber überlegen, wie man das realisieren könnte
- Zeile 131: Das einzelne `raise` soll genau was bringen?
- Zeile 132: Fange niemals *alle* Ausnahmen ab. Fange nur diejenigen ab, die Du wirklich behandeln willst. In dem Falle wohl einen `ValueError`.
- Zeilen 140f.:
Code: Alles auswählen
troubleshoot = True
while troubleshoot:
Code: Alles auswählen
while True:
# usw
- Als letztes: Es fehlt ein wenig Doku und ggf. ein Lizenzhinweis. Zumindest einen Modul-Docstring mit Hinweis auf die Funktionsweise des Moduls könnte man erwarten.
So, das war ziemlich viel... ich hoffe Du kannst da einiges von gebrauchen.
Positiv finde ich übrigens, dass Du Dein Script in einem Pastebin ausgelagert hast
encoding_kapiert = all(verstehen(lesen(info)) for info in (Leonidas Folien, Blog, Folien & Text inkl. Python3, utf-8 everywhere))
assert encoding_kapiert
assert encoding_kapiert
So, hab jetzt endlich etwas Zeit für Python gehabt
Skript: Klick mich!
Ich habe es nur deswegen mit Klassen gemacht, weil ich mir gedacht habe, dass ich eventuell etwas OOP dazulerne ^^
Wie weit soll ich den "mc_desktop_content" einrücken? Bin mir bei solchen kleinigkeiten immer ziemlich unsicher ^.^
z.B.:
Eher so:
oder so:
Ich habe es immer nur mit "if...elif" gemacht
Da ich noch nie (leider) mit solchen Datenstrukturen gearbeitet habe, komme ich gerade überhaupt nicht weiter beim "Troubleshooting-Submenu".
Habs jetzt mal ausgelassen und die anderen Fehler ausgebessert
Hab letzte Woche ziemlichen Stress gehabt, deshalb hat es so lange gedauert bis ich endlich einmal Zeit für Python gefunden habe..
Die Dokumentation füge ich später hinzu wenn alles passt
Würde mich wieder über ein Feedback freuen
mfg
Edit: Hab das Tutorial gar nicht gesehen, werde mein Skript noch einmal Überarbeiten und den Gist updaten
Skript: Klick mich!
Habs jetzt ohne Klassen gemacht, schaut doch gleich viel schöner aus- Wozu diese Klassen? In Wirklichkeit "missbrauchst" Du diese als Modul. Sie haben keinen "inneren" Zustand und kapseln auch nichts. Das könnte man alles eleganter ohne Klassen lösen!
Ich habe es nur deswegen mit Klassen gemacht, weil ich mir gedacht habe, dass ich eventuell etwas OOP dazulerne ^^
An das hab ich garnicht gedacht das man dies mit ".format()" formatieren kann. Hoffentlich hab ich's mir jetzt eingeprägt^^- Es gibt Multiline-Strings (für Zeile 11):Wenn man dabei eine Einrückung zur besseren Lesbarkeit bewahren will, kann man mittels `dedent`-Modul die Einrückungstiefe eleminieren.Code: Alles auswählen
s = """ Hallo Welt! """
- Strings sollte man nicht per `+`-Operator zusammenbauen - auch wenn das natürlich geht. Benutze dafür z.B. die `"".format()`-Methode:Code: Alles auswählen
s = "Hallo, {}".format("Hyperion")
Wie weit soll ich den "mc_desktop_content" einrücken? Bin mir bei solchen kleinigkeiten immer ziemlich unsicher ^.^
z.B.:
Eher so:
Code: Alles auswählen
shutil.copy(mc_desktop, os.path.join(os.environ['HOME'],
"Arbeitsfläche/minecraft.desktop"))
Code: Alles auswählen
shutil.copy(mc_desktop, os.path.join(os.environ['HOME'],
"Arbeitsfläche/minecraft.desktop"))
Wusst ich nicht, ich hab nur gelesen, dass man Dateien mit "with" öffnen soll- Zeile 37: Öffnet man Dateien mit `with`, braucht man diese nicht explizit zu schließen. Das `f.close` kannst (und solltest!) Du Dir sparen.
Hab das Skript so verändert, dass es keine "root"-Rechte benötigt und den Launcher im Home-Verzeichnis des Users (~/.local/share/applications) speichert. Ich finde, dass es die beste Variante ist oder?- Ich weiß nicht genau, ob es eine elegante Möglichkeit gibt, einzelne Abschnitte in einem Python-Programm mit speziellen Rechten auszuführen, aber Deine Idee, `sudo` mit `os.system` zu koppeln klingt für mich wenig sinnvoll. Zum einen ist das sehr plattformspezifisch (Unter Windows wird das alles nicht klappen!), zum anderen wird es schwierig, eine sinnvolle Fehlerbehandlung durchzuführen. Bei Dir fehlt die ja auch komplett. Wenn Du die entsprechenden Funktionen aus den `os`- oder dem `shutil`-Modulen benutzt, wird das schon einfacher zu realisieren. Insofern würde ich dann ggf. für die komplette Ausführung des Programms Administratorrechte "verlangen", aber dafür die Funktionen aus der Standard-Lib benutzen.
- Anstelle von `os.system` solltest Du auch eher auf das `subprocess`-Modul setzen.
Ah danke! Wusste nicht das so ein Menü noch einfacher zu realisieren ist- Dir fehlt es an einer Struktur für Deine Menüeinträge. Was passiert, wenn Du das Menü an zwei Stellen ändern willst und noch Einträge dazu kommen, die dann auch noch an verschiedenen Stellen eingefügt werden sollen? So etwas kann man einfacher lösen, indem man für ein Menü eine passende Datenstruktur benutzt und die Verzweigung dann "berechnet", anstatt sie hart in langen `if...elif`-Kaskaden zu kodieren. (Für diese Sache brauchen wir mal eine wiki-Seite )
Beispiel:Ich habe hier ein Tupel für jeden Menüeintrag benutzt. Als erster Eintrag steht ein String, der den Eintrag beschreibt, als zweites Element füge ich ein Funktionsobjekt hinzu, welches bei Auswahl aufgerufen werden soll. Das ist die "Magie", die für viele Anfänger neu / unbekannt ist. In Python ist alles ein Objekt und deswegen kann ich alles auch an Stellen verwenden, an denen ein Objekt stehen darf. Also auch in Listen, Dictionaries, Tupeln usw.Code: Alles auswählen
main_menu = ( ("Install Minecraft", install), ("Uninstall Minecraft", uninstall), ("Troubleshooting", troubleshoot), ("Exit", lambda: sys.exit(0)), ) for index, item in enumerate(main_menu, 1): print "{} {}".format(index, item[0]) choice = input(...) # here is the 'magic'; we call a function, without using its name. main_menu[choice-1][1]()
Meine Struktur kann ich nun einfach durchgehen und die Ausgabe erzeugen. Die Benutzereingabe habe ich nur mal angedeutet, aber da hast Du ja etwas brauchbares. Bei positiver Verifikation benutzt Du den Index (-1, da Indizies bei 0 anfangen, unsere Ausgabe aber bei 1 anfängt) und pickst den zweiten Eintrag heraus, also die aufzurufende Funktion. Diese rufst Du dann einfach auf `()`. Schon landest Du in den Funktionen, die dann eine bestimmte "Arbeit" durchführen.
Willst Du nun Einträge umsortieren, neue Einträge einfügen o.ä., brauchst Du nur die Menüdefinition anpassen. Ich hoffe Du siehst, dass das viel einfacher ist, als Deine Lösung.
Ok,. Du musst noch ein Submenü einbauen... da lasse ich Dich mal selber überlegen, wie man das realisieren könnte
Ich habe es immer nur mit "if...elif" gemacht
Da ich noch nie (leider) mit solchen Datenstrukturen gearbeitet habe, komme ich gerade überhaupt nicht weiter beim "Troubleshooting-Submenu".
Habs jetzt mal ausgelassen und die anderen Fehler ausgebessert
Falls "choice < 0" oder "choice >= 5" ist soll zum "except" geraised werden- Zeile 131: Das einzelne `raise` soll genau was bringen?
Erledigt- Zeile 132: Fange niemals *alle* Ausnahmen ab. Fange nur diejenigen ab, die Du wirklich behandeln willst. In dem Falle wohl einen `ValueError`.
Wusst ich noch nicht- Zeilen 140f.:Was soll das bewirken? Wozu der Name `troubleshoot`? Du kannst doch genauso gut gleichCode: Alles auswählen
troubleshoot = True while troubleshoot:
schreiben! Du verlässt die Schleife doch eh erst durch ein `break` und nicht etwa dadurch, dass Du `troubleshoot` auf `False` setzt. Zudem bindest Du im Schleifenrumpf ständig neue Werte an `troubleshoot` - ist das wirklich beabsichtigt? Wäre ein Menüeintrag "0", so würdest Du unfreiwillig die Schleife verlassenCode: Alles auswählen
while True: # usw
Herzlichen Dank!! Soviel dazu gelernt hab ich schon lange nicht mehr in PythonSo, das war ziemlich viel... ich hoffe Du kannst da einiges von gebrauchen.
Hab letzte Woche ziemlichen Stress gehabt, deshalb hat es so lange gedauert bis ich endlich einmal Zeit für Python gefunden habe..
Die Dokumentation füge ich später hinzu wenn alles passt
Würde mich wieder über ein Feedback freuen
mfg
Edit: Hab das Tutorial gar nicht gesehen, werde mein Skript noch einmal Überarbeiten und den Gist updaten
Zuletzt geändert von sh4nks am Dienstag 1. Mai 2012, 10:41, insgesamt 1-mal geändert.
Mittlerweile wurde genau dafür ein Tutorial geschrieben: Einfache Textmenüs mit Pythonsh4nks hat geschrieben:Da ich noch nie (leider) mit solchen Datenstrukturen gearbeitet habe, komme ich gerade überhaupt nicht weiter beim "Troubleshooting-Submenu".
Habs jetzt mal ausgelassen und die anderen Fehler ausgebessert
Hab jetzt mein Skript nocheinmal überarbeitet: Klick mich!
Soll ich das Dictionary mit den Menus in die "run"-Funktion geben oder soll ich es so lassen wie es ist?
Verbesserungsvorschläge?
mfg
Soll ich das Dictionary mit den Menus in die "run"-Funktion geben oder soll ich es so lassen wie es ist?
Verbesserungsvorschläge?
mfg
- Hyperion
- Moderator
- Beiträge: 7478
- Registriert: Freitag 4. August 2006, 14:56
- Wohnort: Hamburg
- Kontaktdaten:
Ich würde die Menüdefinition nicht in die `run`-Funktion reinziehen. Man könnte überlegen, die Definition in den `main`-Hook rein zubringen.
Ich habe mir den Rest jetzt nicht en Detail angeguckt, aber ich sehe immer noch ein `f.close`
Ansonsten sieht es auf den ersten Blick besser aus.
Ich habe mir den Rest jetzt nicht en Detail angeguckt, aber ich sehe immer noch ein `f.close`
Ansonsten sieht es auf den ersten Blick besser aus.
encoding_kapiert = all(verstehen(lesen(info)) for info in (Leonidas Folien, Blog, Folien & Text inkl. Python3, utf-8 everywhere))
assert encoding_kapiert
assert encoding_kapiert
- Hyperion
- Moderator
- Beiträge: 7478
- Registriert: Freitag 4. August 2006, 14:56
- Wohnort: Hamburg
- Kontaktdaten:
Das hiersh4nks hat geschrieben:Was meinst du mit "main-Hook"?
Code: Alles auswählen
if __name__ == "__main__":
# entweder man ruft hier eine `main`-Funktion auf
# oder schreibt eben Code direkt hier rein, der das Script "startet"
encoding_kapiert = all(verstehen(lesen(info)) for info in (Leonidas Folien, Blog, Folien & Text inkl. Python3, utf-8 everywhere))
assert encoding_kapiert
assert encoding_kapiert