3
\$\begingroup\$

This program writes out all the files in the current directory level to a text file, outputs an array of sub-directories at the current level, then repeats this process until there are no more sub-directories to search. The code below take 50 seconds to run.

I was hoping I could get any pointers on the code in general (I'm very new to C#) and also any suggestions for improving the runtime.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.IO;

namespace ComputerFileOutput
{
    class Program
    {
        // Prints a string array of files located in specified path
        static void filesArray(string path)
        {
                string[] array = Directory.GetFiles(path);
                addFilesToTextFile(array);
        }

        //Appends files to .txt document
        static void addFilesToTextFile(string[] fileArray)
        {
            //Append fileArray to current .txt document
            using (System.IO.StreamWriter file = new System.IO.StreamWriter(@"C:\ComputerFiles.txt", true))
            {
                for (int i = 0; i < fileArray.Length; ++i)
                {
                    file.WriteLine(fileArray[i]);
                }
            }
        }

        //Returns a string array of directories in a specified path
        static string[] getDirectories(string path)
        {
            return (Directory.GetDirectories(path));
        }


        static void Main(string[] args)
        {
            DateTime t1 = DateTime.Now;
            string[] startUp = { "------LIST OF ALL COMPUTER FILES------" };
            System.IO.File.WriteAllLines(@"C:\ComputerFiles.txt", startUp);

            //Get files in c directory; append to .txt file
            filesArray("C:\\");

            //Get array of primary directories in C drive
            string[] directoryArray = getDirectories("C:\\");

            //initialize subdirectory search
            bool moreDirectories = true;



            //Go through each directory
            while (moreDirectories == true)
            {
                 List<string> subDirectories = new List<string>();
                foreach(string directoryName in directoryArray)
                {
                    try
                    {
                        filesArray(directoryName);

                        subDirectories.AddRange(getDirectories(directoryName));
                    }
                    catch(Exception)
                    {
                        Console.WriteLine("Unathorized to access directory: {0}", directoryName);
                    }
                }

                directoryArray = subDirectories.ToArray();

                if (directoryArray.Count() == 0)
                {
                    moreDirectories = false;
                    break;
                }
            }


            // wait for input before exiting
            DateTime t2 = DateTime.Now;
            Console.WriteLine("Total seconds = {0}", (t2 - t1).TotalSeconds);
            Console.WriteLine("Press enter to finish");
            Console.ReadLine();

        }
    }
}
\$\endgroup\$
  • \$\begingroup\$ what happens to the application when you hit an Exception in your try/catch? does the application stop, or does it skip that one and keep going? \$\endgroup\$ – Malachi Aug 27 '13 at 15:27
  • 2
    \$\begingroup\$ When you say "the code takes 50 seconds to run" - is that in ALL cases, no matter the depth of recursion required, or just for the hard-coded example case of "C:\\" ? I wouldn't be too surprised at 50 seconds for detailing your entire c drive. \$\endgroup\$ – Michael Paulukonis Aug 27 '13 at 15:42
  • 1
    \$\begingroup\$ Try capturing all files, THEN writing them out. Or skipping file io entirely -- debug to view the output and see if the time changes. Try some logging to time the variations. \$\endgroup\$ – Michael Paulukonis Aug 27 '13 at 15:43
  • \$\begingroup\$ I think it's worth pointing out that the whole file enumeration could be done via Directory.GetFiles(@"C:\", "*.*", SearchOption.AllDirectories)... \$\endgroup\$ – Patryk Ćwiek Aug 28 '13 at 12:11
3
\$\begingroup\$

First of all, you shouldn't hardcode values like your C:\ or C:\ComputerFiles.txt. If you want to change this, you don't want to edit it at multiple places.

static void filesArray(string path)

The usual .Net naming convention for methods is PascalCase (you'll notice that Main has to follow this convention).

Also, filesArray is pretty bad name for this method. It doesn't tell you what the method does (that it prints somethings). On the other hand, it tells you about an implementation detail (it uses an array), which it shouldn't.

static void addFilesToTextFile(string[] fileArray)

Whenever you call this method, you open and then close a file. I'm not sure about this, but it could add a measurable overhead, considering that you're going to have huge amount of directories.

return (Directory.GetDirectories(path));

The outer parentheses are not necessary here, remove them.

static void Main(string[] args)

If you're not going to use command-line arguments, you can safely remove the args parameter.

DateTime t1 = DateTime.Now;

To measure runtime of your program accurately, you should usually use Stopwatch instead of DateTime. Though if the time is on the order of 50 s, you don't need millisecond accuracy, so DateTime is probably okay.

while (moreDirectories == true)

This whole loop could be simplified if you used Queue<T>. Something like:

//Get array of primary directories in C drive
var directories = new Queue<string>(getDirectories("C:\\"));

//Go through each directory
while (directories.Count > 0)
{
    string directoryName = directories.Dequeue();

    try
    {
        filesArray(directoryName);

        foreach (var subdirectory in getDirectories(directoryName))
            directories.Enqueue(subdirectory);
    }
    catch (Exception)
    {
        Console.WriteLine("Unathorized to access directory: {0}", directoryName);
    }
}

If you don't like that foreach, you could easily create an extension method for it, called something like EnqueueRange().

catch(Exception)

If you're expecting unauthorized access errors, then you should catch UnauthorizedAccessException, nothing else.

\$\endgroup\$
  • \$\begingroup\$ That should probably be directories.Count in the loop condition of your Queue example. \$\endgroup\$ – Bob Aug 28 '13 at 11:52
2
\$\begingroup\$

Due the recursive nature of the file system and the limited deep of directory tree, you can use a recursive function to do the work. The code will be more expresive and there will be less duplications.

    private static void Main(string[] args) {
        System.Diagnostics.Stopwatch sw = new System.Diagnostics.Stopwatch();
        sw.Start();

        AppendFilesRecursively("c:\\");

        sw.Stop();
        Console.WriteLine("Total seconds = {0}", sw.Elapsed.TotalSeconds);


    }

    private static void AppendFilesRecursively(string directoryName) {
        string[] directoryArray;
        try {
            filesArray(directoryName);
            directoryArray = getDirectories(directoryName);
        }
        catch (UnauthorizedAccessException) {
            Console.WriteLine("Unathorized to access directory: {0}", directoryName);
            return;
        }
        foreach (var directory in directoryArray) {
            AppendFilesRecursively(directory);
        }
    }
\$\endgroup\$
1
\$\begingroup\$

This looks like a job for Separation of Concerns. Split up into four classes - one which can gather all directory names given a root path, one which can gather up files for a given directory path, one which integrates the two, gathering all files for all directories and finally, one which can send those to an output file. Also interfaced for unit testability.

public interface IDirectoryGatherer
{
    TextWriter Writer
    {
        get;
    }

    IEnumerable<string> Directories
    {
        get;
    }

    void Gather();
}

public sealed class DirectoryGatherer : IDirectoryGatherer
{
    private readonly string path;

    private readonly TextWriter writer;

    private readonly List<string> directories = new List<string>();

    public DirectoryGatherer(string path, TextWriter writer)
    {
        this.path = path;
        this.writer = writer;
    }

    public TextWriter Writer
    {
        get
        {
            return this.writer;
        }
    }

    public IEnumerable<string> Directories
    {
        get
        {
            return this.directories;
        }
    }

    public void Gather()
    {
        try
        {
            this.directories.AddRange(Directory.GetDirectories(this.path));
        }
        catch (UnauthorizedAccessException)
        {
            this.writer.WriteLine("Unauthorized to access directory: {0}", this.path);
        }
        catch (DirectoryNotFoundException)
        {
            this.writer.WriteLine("Directory {0} not found", this.path);
        }

        var gatherers = this.directories.Select(directory => new DirectoryGatherer(directory, this.writer)).ToList();

        for (var i = 0; i < gatherers.Count; i++)
        {
            gatherers[i].Gather();
            this.directories.AddRange(gatherers[i].Directories);
        }
    }
}

public interface IFileGatherer
{
    IEnumerable<string> Files
    {
        get;
    }

    void Gather();
}

public sealed class FileGatherer : IFileGatherer
{
    private readonly string path;

    private readonly TextWriter writer;

    private string[] files;

    public FileGatherer(string path, TextWriter writer)
    {
        this.path = path;
        this.writer = writer;
    }

    public IEnumerable<string> Files
    {
        get
        {
            return this.files ?? Enumerable.Empty<string>();
        }
    }

    public void Gather()
    {
        try
        {
            this.files = Directory.GetFiles(this.path);
        }
        catch (UnauthorizedAccessException)
        {
            this.writer.WriteLine("Unauthorized to access directory: {0}", this.path);
        }
        catch (DirectoryNotFoundException)
        {
            this.writer.WriteLine("Directory {0} not found", this.path);
        }
    }
}

public interface IFilesCollection
{
    IEnumerable<string> Files
    {
        get;
    }

    void Gather();
}

public sealed class FilesCollection : IFilesCollection
{
    private readonly IDirectoryGatherer directoryGatherer;

    private readonly List<string> files = new List<string>();

    public FilesCollection(IDirectoryGatherer directoryGatherer)
    {
        this.directoryGatherer = directoryGatherer;
    }

    public IEnumerable<string> Files
    {
        get
        {
            return this.files;
        }
    }

    public void Gather()
    {
        this.directoryGatherer.Gather();
        foreach (var fileGatherer in this.directoryGatherer.Directories.Select(directory => new FileGatherer(directory, this.directoryGatherer.Writer)))
        {
            fileGatherer.Gather();
            this.files.AddRange(fileGatherer.Files);
        }
    }
}

public interface IStringsWriter
{
    void Write();
}

public sealed class StringsWriter : IStringsWriter
{
    private readonly string fileName;

    private readonly IEnumerable<string> strings;

    public StringsWriter(string fileName, IEnumerable<string> strings)
    {
        this.fileName = fileName;
        this.strings = strings;
    }

    public void Write()
    {
        // Append fileArray to current .txt document
        using (var file = new StreamWriter(this.fileName, true))
        {
            foreach (var s in this.strings)
            {
                file.WriteLine(s);
            }
        }
    }
}

internal static class Program
{
    private static void Main()
    {
        var t1 = Stopwatch.StartNew();

        File.WriteAllLines(@"C:\ComputerFiles.txt", new[] { "------LIST OF ALL COMPUTER FILES------" });

        // Get files in c directory; append to .txt file
        // Get array of primary directories in C drive
        IDirectoryGatherer directoryGatherer = new DirectoryGatherer(@"C:\", Console.Out);
        IFilesCollection allFiles = new FilesCollection(directoryGatherer);

        allFiles.Gather();

        IStringsWriter writer = new StringsWriter(@"C:\ComputerFiles.txt", allFiles.Files);

        writer.Write();

        // wait for input before exiting
        Console.WriteLine("Total seconds = {0}", t1.Elapsed.TotalSeconds);
        Console.WriteLine("Press enter to finish");
        Console.ReadLine();
    }
}
\$\endgroup\$
1
\$\begingroup\$

Open console and type:

TREE C:\

To see not only dirs but files:

TREE /F C:\
\$\endgroup\$
0
\$\begingroup\$

You probably want to set the folder and file names to be constants somewhere.

I didn't worry to much about performance, besides throwing exceptions it ran pretty fast on my computer.

All I tried to do here is simplify your main loop so that all the initialization, testing for termination, etc. were on one line.

Often flags and break statements are not needed, and the code becomes shorter and cleaner:

List<string> subDirectories = new List<string>();
for (string[] directoryArray = getDirectories("C:\\"); 
     directoryArray.Count() > 0;  directoryArray = subDirectories.ToArray())
{
    foreach (string directoryName in directoryArray)
    {
         processSubFolder(subDirectories, directoryName);
    }
}

// wait for input before exiting

Also I extracted this helper method:

private static void processSubFolder(List<string> subDirectories, string directoryName)
{
    try
    {
        filesArray(directoryName);
        subDirectories.AddRange(getDirectories(directoryName));
    }
    catch (Exception)
    {
        Console.WriteLine("Unathorized to access directory: {0}", directoryName);
    }
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.