Code-Review - Erstellen einer Datei- und Ordnerliste

JackCoder · Hält's aus hier Beiträge: 7

Hallo,
ich habe ein Programm geschrieben, das aus einem gegeben Ordner alle Dateien und Unterordner in eine Datei auflistet. Zudem wird zu jeder Datei ein SHA512-Code generiert.
Hier erst einmal der Quellcode:

markieren

C#-Quelltext

1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:

    public static void CreateFileList(string pathFileList, string pathMainDirectory) {

        if(progress == 1.0) {

            progress = 0.0;

            Thread thread = new Thread(delegate() {

                double progressStep = 1.0 / Directory.EnumerateFiles(pathMainDirectory, "*", SearchOption.AllDirectories).Count();
                File.WriteAllText(pathFileList, string.Empty);
                Action<string> writeFileList = null;

                writeFileList = delegate(string pathCurrentDirectory) {

                    IEnumerable<string> pathsSubdirectories = Directory.EnumerateDirectories(pathCurrentDirectory);
                    List<string> data = new List<string>();
                    data.AddRange(from pathCurrentSubdirectory in pathsSubdirectories
                                  select Path.GetFileName(pathCurrentSubdirectory));

                    foreach(FileInfo currentFile in (new DirectoryInfo(pathCurrentDirectory)).EnumerateFiles()) {

                        if(!currentFile.Name.Equals(Path.GetFileName(pathFileList))) {

                            data.Add(currentFile.Name + Path.DirectorySeparatorChar.ToString() + FileGetHashCode(currentFile.FullName));
                            progress += progressStep;

                        }

                    }

                    File.AppendAllLines(pathFileList, data);

                    foreach(string pathCurrentSubdirectory in pathsSubdirectories) {

                        File.AppendAllText(pathFileList, Environment.NewLine + pathCurrentSubdirectory.Replace(pathMainDirectory, string.Empty) + Environment.NewLine);
                        writeFileList(pathCurrentSubdirectory);

                    }

                };

                writeFileList(pathMainDirectory);

                if(progress != 1.0)
                    progress = 1.0;

            });

            thread.IsBackground = true;
            thread.Start();

        } else {

            throw new InvalidOperationException("Program is still performing another operation");

        }

    }

Die Methode befindet sich in einer Klasse mit der Eigenschaft progress (als double von 0 bis 1; dieser wird mit 100 multipliziert und als Integer zurückgegeben), mit der der aktuelle Fortschritt dargestellt wird.
Ich habe zum ersten mal mit Delegates gearbeitet, was haltet ihr davon? Ich lese die Unterordner mit Rekursion aus und das in einem Thread, deshalb die Delegates.
Könnt ihr mir Verbesserungsvorschläge geben oder ist das so in Ordnung, Danke!

Palladin007 · Verfasst: So 25.05.14 22:11

Ich habe keinen Schimmer, was die Methode tut.
Das einzige, was ich weiß, ist das, was du geschrieben hast, für mehr Informationen müsste ich erst lang und breit mit ich die Methode rein denken und das sollte so nicht sein.
Ich würde dafür eine ganze Klasse schreiben, die dann über Eigenschaften den aktuellen Stand angibt und über Methoden den Background-Thread steuern lässt.
Intern ist dann der Inhalt auf mehrere Methoden aufgeteilt, die kleinteilig ihre Aufgaben übernehmen, dafür aber jede schnell und leicht verständlich ist.

Ich versuche gerade, das zu ordnen und bisher merke ich, dass sich das ganz stark aufdröseln lässt.

Btw: Warum hast du die Variable writeFileList verwendet? Warum nicht einfach als eigene Methode? Wäre viel leichter zu lesen.

Ralf Jansen · Verfasst: So 25.05.14 22:47

Zitat:

Das einzige, was ich weiß, ist das, was du geschrieben hast, für mehr Informationen müsste ich erst lang und breit mit ich die Methode rein denken und das sollte so nicht sein.

Wer den Kernighan-Ritchie Stil beim setzen von geschweiften Klammern benutzt will auch gar nicht das man das einfach lesen und verstehen kann. Brauchst dich also nicht zu wundern

Bitte, wer nicht gerade c oder Javascript programmiert macht das nie nie nimmmer nicht. :flehan:

Zum Code.Du hast mehr als einen Schreibvorgang in die Datei hinter pathFileList. Also öffne einen Filestream halte den offen und hänge da neue Daten jeweils an. Die ~scheinbar~ einfach machenden Methoden der File Klasse öffnen und schließen das File bei jedem Schreibvorgang das macht es a.) elendig langsam (was einem je nach Anwendung noch egal sein kann) und b.) leben wir in einer Zeit mit einer Krankheit die sich Virenscanner nennt. Der Großteil der vorhandenen Virenscanner reagiert empfindlich auf sowas da die bei jedem Schreibvorgang anspringen. Schnelles öffnen/schließen/öffnen etc. ist heutzutage ein absolutes NoGo wenn man eine halbwegs stabile Anwendung möchte die sich mit vorhandene Virenscanner vertragen soll.

Palladin007 · Verfasst: Mo 26.05.14 00:00

So, habs jetzt mal lesbarer gemacht. Zufrieden bin ich nicht, mir ist aber auch nur halb klar, was da genau passieren soll.
Zum Thema Multithreading kenne ich bisher auch nur die Theorie, von daher rechne ich mit Patzern.
Und getestet ist es auch nicht, ist mir zu spät ^^

markieren

C#-Quelltext

1:
2:
3:
4:
5:
6:
7:
8:
9:
10:
11:
12:
13:
14:
15:
16:
17:
18:
19:
20:
21:
22:
23:
24:
25:
26:
27:
28:
29:
30:
31:
32:
33:
34:
35:
36:
37:
38:
39:
40:
41:
42:
43:
44:
45:
46:
47:
48:
49:
50:
51:
52:
53:
54:
55:
56:
57:
58:
59:
60:
61:
62:
63:
64:
65:
66:
67:
68:
69:
70:
71:
72:
73:
74:
75:
76:
77:
78:
79:
80:
81:
82:
83:
84:
85:
86:
87:
88:
89:
90:
91:
92:
93:
94:
95:
96:
97:
98:
99:
100:
101:
102:
103:
104:
105:
106:
107:
108:
109:
110:
111:
112:
113:
114:
115:
116:
117:
118:
119:
120:
121:
122:
123:
124:
125:
126:
127:
128:

public class FileListCreater : IDisposable
{
    private readonly Thread _workerThread;
    private string[] _files;
    private double _progessStep;
    private TextWriter _targetWriter;
    private string _mainDirectory;

    public TextWriter TargetWriter
    {
        get { return _targetWriter; }
        set
        {
            ThrowIfIsRunning();
            _targetWriter = TextWriter.Synchronized(value);
        }
    }
    public string MainDirectory
    {
        get { return _mainDirectory; }
        set
        {
            ThrowIfIsRunning();
            _mainDirectory = value;
        }
    }
    public double Progress { get; private set; }
    public bool IsRunning
    {
        get { return _workerThread.ThreadState == ThreadState.Running; }
    }

    public FileListCreater()
    {
        ParameterizedThreadStart workMethod = parameter => WriteFileList((string)parameter);
        _workerThread = new Thread(workMethod);
    }
    ~FileListCreater()
    {
        Dispose(false);
    }

    public void Start()
    {
        ValidateForStart();
        ThrowIfIsRunning();

        Progress = 0;
        _files = Directory.EnumerateFiles(_mainDirectory, "*", SearchOption.AllDirectories).ToArray();
        _progessStep = 1.0 / _files.Length;

        _workerThread.Start();
    }
    public void Reset()
    {
        _workerThread.Abort();
        _files = null;
        _progessStep = 0;
        _targetWriter = null;
        _mainDirectory = null;
        Progress = 0;
    }

    private void ValidateForStart()
    {
        if (TargetWriter == null)
            throw new InvalidOperationException("TargetWriter is null");
        if (string.IsNullOrWhiteSpace(MainDirectory))
            throw new InvalidOperationException("MainDirectory is empty");
    }
    private void ThrowIfIsRunning()
    {
        if (IsRunning)
            throw new ThreadStateException("the thread is running");
    }
    private void WriteFileList(string directory)
    {
        var subDirectories = Directory.EnumerateDirectories(directory);
        var files = new DirectoryInfo(directory).EnumerateFiles();

        foreach (var subDirectory in subDirectories)
            TargetWriter.WriteLine(Path.GetFileName(subDirectory));

        foreach (var file in files)
        {
            TargetWriter.WriteLine(Path.Combine(file.FullName, GetFileHashCode(file.FullName)));
            Progress += _progessStep;
        }

        foreach (var subDirectory in subDirectories)
        {
            TargetWriter.WriteLine();
            TargetWriter.WriteLine(subDirectory.Replace(directory, string.Empty));
            TargetWriter.WriteLine();

            WriteFileList(subDirectory);
        }
    }
    private string GetFileHashCode(string file)
    {
        byte[] sha512Hash;

        using (var fileStream = File.OpenRead(file))
            sha512Hash = new SHA512CryptoServiceProvider().ComputeHash(fileStream);

        return BitConverter.ToString(sha512Hash);
    }

    public void Close()
    {
        ((IDisposable)this).Dispose();
    }
    void IDisposable.Dispose()
    {
        Dispose(true);
        GC.SuppressFinalize(this);
    }
    protected virtual void Dispose(bool disposing)
    {
        if (disposing)
        {
            Reset();

            if (_targetWriter != null)
                _targetWriter.Dispose();
        }
    }
}

	Mitgliederliste
	Gruppen
	Das Team
	Richtlinien
	Synonyme