@snowflake: Ich würde nicht sagen das ist objektorientiert. Das ist als Klasse implementiert, aber ich sehe da nicht so wirklich einen Grund dafür das als Klasse zu implementieren.
`os.path` braucht man nicht importieren, das macht das importieren des `os`-Moduls schon.
Das Hauptprogramm sollte in einer Funktion stehen die üblicherweise `main()` heisst.
`DelFiles` enthält eine Abkürzung und `DeleteFiles` klingt eher nach einer Tätigkeit, Klassen beschreiben aber ”Dinge” im weitesten Sinne. Das wäre dann also eher `FileDeleter`. Und ein Objekt davon sollte man nicht `a` nennen.
Kommentare sollten nicht beschreiben *was* gemacht wird, denn das steht da bereits als Code sondern *warum* der Code das so macht wie er das macht. Sofern das nicht offensichtlich ist.
Die Kommentare über den ”Methoden” wären besser Docstrings.
Namen werden in Python klein_mit_unterstrichen geschrieben. Ausnahmen sind Konstanten (KOMPLETT_GROSS) und Klassen (MixedCase). Die Unterstriche sind auch wichtig, denn man sollte den Leser nicht damit belasten mehrerenahtloszusammengeschriebene Worte jedesmalim Kopf auseinanderpflückenzumüssen.
Man vergleicht nicht mit literalen Wahrheitswerten. Da kommt doch entweder der Wahrheitswert heraus den man sowieso schon hatte, oder das Gegenteil davon. Im ersten Fall kann man den Wert direkt nehmen, im zweiten Fall kann man ihn mit ``not`` negieren.
`os.path.exists()` gibt bereits einen Wahrheitswert zurück, da macht es keinen Sinn ein komplettes ``if``/``else`` hinzuschreiben das ”entscheided”, dass man den gleichen Wert zurück gibt:
Code: Alles auswählen
def pruefe_pfad(self):
"""
Überprüft, ob der angegebene Pfad vorhanden ist.
"""
if os.path.exists(self.pfad):
return True
else:
return False
# =>
def pruefe_pfad(self):
"""
Überprüft, ob der angegebene Pfad vorhanden ist.
"""
return os.path.exists(self.pfad)
Und hier fragt man sich dann ob das überhaupt eine ”Methode” wert ist, denn `pruefe_pfad()` ist ja eigentlich nur ein umbenanntes `os.path.exists()`.
Die API mit den `True`/`False` Rückgabewerten wenn es (keine) Probleme gab ist doof. Dafür gibt es Ausnahmen. Da kann man dann nicht nur Kommunizieren *das* etwas nicht geklappt hat, sondern auch *was* das war. Ausnahmen in `False`-Rückgabewerte zu übersetzen ist ein Warnzeichen. Die Programmlogik sollte auch keine Benutzerinteraktion betreiben.
Dadurch dass das alles sinnlos auf ”Methoden” verteilt ist und alle Listen ”Zustand” dieses Objekts sind, wird der Code unübersichtlicher weil eigentlich lokale Variablen jetzt von mehreren ”Methoden” geteilt werden. `self.zu_loeschende_dateien` in `finde_dateien()` auf eine leere Liste zu setzen gehört da eigentlich nicht hin, denn das hat ja nichts mit dem Finden von Dateien zu tun. Ist zudem redundant, denn die aufrufende Methode macht das ja selbst auch noch einmal.
Was die `finde_dateien()` dann macht ist das hier:
Code: Alles auswählen
self.alle_dateien = []
for datei in os.walk(self.pfad):
self.alle_dateien.append(datei)
Was aber man kürzer so schreiben kann:
Und da kann man sich dann auch wieder fragen ob man dafür eine eigene ”Methode” braucht. Das kann man auch direkt beim Aufrufer einfach hinschreiben. Und dann wären wir bei dem Punkt angelangt wo die ”Klasse” nur noch aus einer `__init__()` und einer weiteren ”Methoden” besteht, und die Verwendung daraus das man ein Objekt davon erstellt und darauf dann sofort die einzige Methode aufruft: das ist also einfach eine Funktion die ohne Grund in eine Klasse gesteckt wurde:
Code: Alles auswählen
class FileDeleter:
def __init__(self, pfad, dateityp):
self.pfad = pfad
self.dateityp = dateityp
self.alle_dateien = []
self.zu_loeschende_dateien = []
self.geloeschte_dateien = []
def loesche_dateien(self):
"""
Löscht Dateien mit dem entsprechenden Dateitpy im angegebenen Pfad
"""
#
# Trigger FileNotFound etc. because `os.walk` simply ignores this
# without `onerror`.
#
os.stat(self.pfad)
self.alle_dateien = list(os.walk(self.pfad))
self.zu_loeschende_dateien = []
for inhalt in self.alle_dateien[0][2]:
if "." in inhalt:
endung = inhalt.split(".")[1]
if endung == self.dateityp:
self.zu_loeschende_dateien.append(inhalt)
self.geloeschte_dateien = []
for zeile in self.zu_loeschende_dateien:
pfad1 = self.pfad + "/" + zeile
if os.name == "nt": # Windows
os.chmod(pfad1, stat.S_IWRITE)
os.remove(pfad1)
self.geloeschte_dateien.append(zeile)
elif os.name == "posix": # Linux
os.remove(self.pfad)
Als Funktion:
Code: Alles auswählen
def loesche_dateien(pfad, dateityp):
"""
Löscht Dateien mit dem entsprechenden Dateitpy im angegebenen Pfad
"""
#
# Trigger FileNotFound etc. because `os.walk` simply ignores this
# without `onerror`.
#
os.stat(pfad)
alle_dateien = list(os.walk(pfad))
zu_loeschende_dateien = []
for inhalt in alle_dateien[0][2]:
if "." in inhalt:
endung = inhalt.split(".")[1]
if endung == dateityp:
zu_loeschende_dateien.append(inhalt)
geloeschte_dateien = []
for zeile in zu_loeschende_dateien:
pfad1 = pfad + "/" + zeile
if os.name == "nt": # Windows
os.chmod(pfad1, stat.S_IWRITE)
os.remove(pfad1)
geloeschte_dateien.append(zeile)
elif os.name == "posix": # Linux
os.remove(pfad)
`os.walk()` ist hier völliger Irrsinn, denn es wird von der Liste mit allen Verzeichnis- und Dateinamen unterhalb vom gegebenen `pfad` nur die Dateinamen direkt im `pfad` verwenet. Warum baust Du dann erst die ganze Liste auf? Die kann potentiell richtig gross werden und viel Zeit zum ermitteln benötigen. Das macht keinen Sinn. Was Du da machen willst ist ein einfaches `os.listdir()` und prüfen ob es eine Datei ist, oder `os.scandir()`.
Code: Alles auswählen
alle_dateien = [
entry.name for entry in os.scandir(pfad) if entry.is_file()
]
Und `alle_dateien` enthält dann wirklich nur die Dateinamen die direkt in `pfad` liegen. `os.scandir()` löst eine Ausnahme aus wenn der Pfad nicht existiert, man kann sich den `os.stat()`-Hack also auch sparen.
`inhalt` ist kein guter Name für einen Dateinamen.
Pfade sind keine beliebigen Zeichenketten, die sollte man nicht mit Zeichenkettenoperationen verarbeiten die nichts von den besonderheiten von Pfaden wissen. Um die Dateiendung zu isolieren gibt es beispielsweise `os.path.splitext()`.
Code: Alles auswählen
zu_loeschende_dateien = []
for inhalt in alle_dateinamen:
if "." in inhalt:
endung = inhalt.split(".")[1]
if endung == dateityp:
zu_loeschende_dateien.append(inhalt)
# =>
zu_loeschende_dateien = []
for dateiname in alle_dateinamen:
_, endung = os.path.splitext(dateiname)
if endung == dateityp:
zu_loeschende_dateien.append(dateiname)
Letztlich erfindet man hier aber das Rad neu, denn nach Dateien mit einer Endung suchen gibt es schon im `glob`-Modul in der Standardbibliothek, womit die Funktion auf folgendes zusammenschrumpft:
Code: Alles auswählen
def loesche_dateien(pfad, glob_muster):
"""
Löscht Dateien mit dem entsprechenden Dateitpy im angegebenen Pfad
"""
zu_loeschende_dateien = (
datei_pfad
for datei_pfad in iglob(iglob(os.path.join(pfad, glob_muster)))
if os.path.isfile(datei_pfad)
)
geloeschte_dateien = []
for zeile in zu_loeschende_dateien:
if os.name == "nt": # Windows
os.chmod(zeile, stat.S_IWRITE)
os.remove(zeile)
geloeschte_dateien.append(zeile)
elif os.name == "posix": # Linux
os.remove(pfad)
Nun zum Löschen. `geloeschte_dateien` wird nirgends verwendet und bei POSIX-Systemen auch gar nicht befüllt → weg damit.
`zeile` ist genau wie `inhalt` kein guter Name für einen Dateinamen.
Wenn `glob` nicht bereits komplette Pfade liefern würde, dann wäre es falsch den endgültigen Pfad mit ``+`` und ``"/"`` zusammenzusetzen. Dafür gibt es `os.path.join()`.
Die Funktion macht ohne einen Mucks von sich zu geben einfach *gar nichts* wenn `os.name` weder "nt" noch "posix" ist. Eigentlich soll doch hier nur Windows besonders behandelt werden und gelöscht werden soll in jedem Fall, also braucht man auch nur auf "nt" prüfen und das machen was man da machen will und das `os.remove()` nach dem ``if``-Konstrukt *einmal* hinschreiben. Zudem wird in dem Code unter "posix" versucht den *Pfad* zu löschen. Eine Sache die leicht passiert wenn man Namen wie `pfad` und `pfad1` verwendet.
Zwischenstand bei der Funktion:
Code: Alles auswählen
def loesche_dateien(pfad, glob_muster):
"""
Löscht Dateien mit dem entsprechenden Dateitpy im angegebenen Pfad
"""
zu_loeschende_dateien = (
datei_pfad
for datei_pfad in iglob(os.path.join(pfad, glob_muster))
if os.path.isfile(datei_pfad)
)
for datei_pfad in zu_loeschende_dateien:
if os.name == "nt": # Windows
os.chmod(datei_pfad, stat.S_IWRITE)
os.remove(datei_pfad)
Das `zu_loeschende_dateien` würde ich dann noch mit in die Schleife rein ziehen, dann wird es noch etwas kürzer und übersichtlicher (ungetestet):
Code: Alles auswählen
#!/usr/bin/env python3
"""
Dieses Programm löscht Dateien mit einer bestimmten Endung in einem Verzeichnis.
"""
import os
import stat
from glob import iglob
def loesche_dateien(basis_pfad, glob_muster):
"""
Löscht Dateien mit dem entsprechenden Muster im angegebenen Pfad
"""
for pfad in iglob(os.path.join(basis_pfad, glob_muster)):
if os.path.isfile(pfad):
if os.name == "nt": # Windows
os.chmod(pfad, stat.S_IWRITE)
os.remove(pfad)
def main():
try:
loesche_dateien("C:/temp/testverzeichnis/", "*.JPG")
except OSError as error:
#
# TODO Hier eine Möglichkeit schaffen zu Debug-Zwecken den kompletten
# Traceback auszugeben, zum Beispiel per `logging` statt `print()`.
#
print(error)
else:
print("Keine Fehler beim Löschen...")
if __name__ == "__main__":
main()
Wenn man das objektorientierter schreiben möchte, sollte man IMHO als erstes mal von `os.path` weg kommen und `pathlib` verwenden. Ansonsten ist das einfach kein Problem bei dem man selbst einen Datentyp schreiben muss/will. Klassen sind kein Selbstzweck und durch eine Klasse wird ein Programm nicht auf magische Weise objektorientiert.
). Da muss ich wohl noch sehr viel lernen.