Table_Analyzer

[headhunter1978]

Hallo zusammen,

ich habe gestern ein kleine Tool geschrieben, mit dessen Hilfe ich eine csv-Datei einlese und sortieren kann. Das Ergebnis wird dann in verschiedene Dateien gespeichert.

Das Skript funktioniert soweit, würde mich trotzdem freuen, wenn ich euch das mal anschaut mir ein konstruktives Feedback geben würde was man besser machen kann.

main.py

Code: Alles auswählen

import argparse
from analyze_table import Table

#CSV-Datei laden und Klasse Table übergeben
class LoadFile:
    def __init__(self, input_file, directory, column):
        try:
            # If the file does not exist,
            # then it would throw an IOError

            file = open(input_file, 'r')
            Table(file, directory, column)
            file.close()

        except IOError:
            # print(os.error) will <class 'OSError'>
            print('Problem reading: ' + input_file)


#Bereitstellen der Eingangsparameter
if __name__ == "__main__":

    parser = argparse.ArgumentParser()
    parser.add_argument("-f", "--file",
                        help="CSV-File to import. Example: /Users/jorge/Desktop/Liste.csv", type=str)
    parser.add_argument("-d", "--directory",
                        help="Target-Directory for export. Example: /Users/jorge/Desktop", type=str)
    parser.add_argument("-c", "--column",
                        help="Column to sort table", type=int)
    args = parser.parse_args()

    LoadFile(args.file, args.directory, args.column)

analyze_table.py

Code: Alles auswählen

import os


class Table:
    def __init__(self,file,directory, column):
        self.create_export_files(file, directory, column)

    def create_export_files(self, file, directory, column):
        print('create_export_files')
        i = 0
        title = ''
        cache = ''
        filename = ''
        
        #Zeilenweise csv-Datei durchgehen
        for line0 in file:
            #Zeilenumbrüche löschen
            line1 = line0.split('\n')[0]
            
            #Spaltenüberschriften bestimmen
            if i == 0:
                title = line1

            if i > 0:
                #Cache füllen mit Sortierkriterium
                if cache != line1.split(';')[column]:
                    cache = line1.split(';')[column]
                    filename = cache + '.csv'
                    
                    #Datei anlegen mit Spaltenüberschrift
                    if not os.path.isfile(filename):
                        export_file = open(directory+'/'+filename, 'w')
                        export_file.write(title +'\n')
                        export_file.close()
                
                #Datei befüllen
                export_file = open(directory+'/'+filename, 'a')
                export_file.write(line0)
                export_file.close()
            i += 1

[__blackjack__]

@headhunter1978: Beide Klassen sind keine Klassen sondern Funktionen die sinnlos in eine Klasse gesteckt wurden. Selbst wenn es Klassen wären, sehe ich keinen Grund das bisschen Code auf zwei Module zu verteilen. Bei zwei Funktionen schon mal gar nicht.

Wobei es drei Funktionen sein sollten, denn der Code im ``if __name__ …``-Zweig erzeugt globale Variablen und gehört deshalb in eine Funktion damit er das nicht mehr tut.

Kommandozeilen-Optionen heissen Optionen weil sie *optional* sind. Keine der drei ”Optionen” in Deinem Programm sind das, die müssen zwingend angegeben werden. Also sind das Argumente und keine Optionen.

Dateien öffnet man am besten in Verbindung mit der ``with``-Anweisung.

`load_file()` macht deutlich mehr als nur eine Datei zu laden. Wobei im ganzen Programm keine Datei tatsächlich in den Speicher geladen wird. Die Funktion würde ich weg lassen.

Statt nur zu auszugeben das es Probleme beim Lesen der Eingabedatei gab, möchte der Leser vielleicht ganz gerne wissen was denn das Problem war. Zudem weisst Du doch gar nicht das/ob die Ausnahme bei der Eingabedatei passiert ist. Der `IOError` kann auch bei einer Ausgabedatei aufgetreten sein, denn das schreiben der Ausgabe wird in dem ``try``-Block ja auch aufgerufen. Das Ziel kann voll sein, nicht existieren, oder der Benutzer könnte keine Schreibrechte haben.

Das Vorbelegen von Variablen mit Dummywerten die dann letztlich gar nicht verwendet werden macht keinen Sinn. Die initialen Werte für `title` und `filename` werden nicht verwendet.

`line0` und `line1` sind mit der Nummerierung nicht nur unschön, sondern machen auch gar keinen Sinn. In `line1` ist im Gegensatz zu `line0` der Zeilenende am Ende entfernt, aber an der Stelle wo `line1` dann in eine Datei geschrieben wird, da wird wieder ein Zeilenende angehängt. Das hätte man sich also sparen können.

`i` ist überflüssig. Statt einen Zähler mitzuführen nur um für die allererste Zeile etwas anders zu machen als für den Rest, ist es einfach, nun ja mit der allerersten Zeile vor der Schleife etwas anders zu machen als für den Rest. So ganz ohne Zähler und ``if``.

Wenn der erste Wert in der gewählten `column` leer ist, dann wird `filename` nicht definiert und das ganze fährt gegen die Wand. `cache` muss einen Wert haben, der garantiert nicht in den Daten vorkommen kann. `None` böte sich hier an. Der Name ist auch nicht gut, einen einzelnen Wert würde ich nicht als `cache` bezeichnen. Das wäre eher so etwas wie `previous_value`. Und den aktuellen Wert kann man dann an `value` binden, statt das Aufteilen der Zeile unnötigerweise zweimal zu machen. Wobei das eigentlich überhaupt gar nicht wirklich nötig ist. Man kann einfach für jeden Wert den Dateinamen erstellen. Sollte das ein messbares Problem mit der Laufzeit werden, *dann* könnte man einen Cache anlegen der den Namen auch verdient: Ein Wörterbuch das Werte auf Dateinamen abbildet.

Pfadbestandteile setzt man mit `os.path.join()` zusammen und nicht mit ``+``.

Du hast das anscheinend nie mit einem anderen Verzeichnis als dem versucht, von dem aus das Programm aufgerufen wird, denn bei `isfile()` verwendest Du nur den Dateinamen ohne den Pfad davor.

Das mit dem schreiben der Kopfzeile in eine neue Datei ist auch etwas umständlich organisiert. Das kann man kürzer schreiben, mit nur einmal Datei öffnen, das man ja sowieso schon im Code hat.

Da bleibt dann als Zwischenstand das hier übrig (ungetestet):

Code: Alles auswählen

#!/usr/bin/env python3
import argparse
import os


def create_export_files(lines, directory, column):
    print('create export files...')
    
    header = next(lines)
    for line in lines:
        value = line.split(';')[column]
        filename = os.path.join(directory, value.rstrip('\n') + '.csv')
        file_is_new = not os.path.isfile(filename)
        with open(filename, 'a') as export_file:
            if file_is_new:
                export_file.write(header)
            export_file.write(line)


def main():
    parser = argparse.ArgumentParser()
    parser.add_argument(
        'file',
        help='CSV-File to import. Example: /Users/jorge/Desktop/Liste.csv',
    )
    parser.add_argument(
        'directory',
        help='Target-Directory for export. Example: /Users/jorge/Desktop',
    )
    parser.add_argument('column', help='Column to sort table', type=int)
    arguments = parser.parse_args()

    try:
        with open(arguments.file, 'r') as file:
            create_export_files(file, arguments.directory, arguments.column)
    except OSError as error:
        print(error)


if __name__ == '__main__':
    main()

Allerdings kann das nicht mit CSV-Dateien umgehen. *Dafür* gibt es das CSV-Modul in der Standardbibliothek. Das CSV-Format ist zwar relativ simpel aber nicht *so* simpel wie Du das verarbeitest.

Das Programm sortiert auch nicht. Das ist irreführend das Kommentare und Hilfe davon sprechen.

[headhunter1978]

danke für das Feedback