4
\$\begingroup\$

I need to get a list of all files supported by some libary (images) in a specified directory and all subderictories. Root directory of drive might be chosen and in this case I want to skip system folders like "Windows" and "AppData" (some other?).

For such task I can use Directory.GetFiles() but problem is - it will stop after getting any exception. In this case I need to just ignore noncritical exceptions and skip folders that cause this exception.

This code looks reasonable to me. But I want to make sure that it is done right.

Please tell me if it should be changed somehow.

public static List<string> FindSupportedFiles(string root)
{
    // class from library 
    var optimizer = new ImageOptimizer();

    Queue<string> folders = new Queue<string>();
    List<string> files = new List<string>();

    folders.Enqueue(root);
    while (folders.Count != 0)
    {
        string currentFolder = folders.Dequeue();

        if (currentFolder.Contains(":\\Windows") ||
             (currentFolder.Contains(":\\Users") && currentFolder.Contains("\\AppData")))
        {
            if (folders.Count != 0) currentFolder = folders.Dequeue();
            continue;
        }

        string[] filesInCurrent = null;
        try
        {
            filesInCurrent = Directory.GetFiles(currentFolder, "*.*");
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }
        catch (DirectoryNotFoundException) { }

        foreach (string f in filesInCurrent)
            if (optimizer.IsSupported(f)) files.Add(f);

        string[] foldersInCurrent = null;
        try
        {
            foldersInCurrent = Directory.GetDirectories(currentFolder);
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }
        catch (DirectoryNotFoundException) { }

        foreach (string _current in foldersInCurrent)
            folders.Enqueue(_current);
    }
    return files;
}
\$\endgroup\$
  • 1
    \$\begingroup\$ Please add the code directly from your IDE. This ensures that the code is correct, at least regarding the scope of your variables. \$\endgroup\$ – Heslacher Jan 23 '18 at 6:18
  • \$\begingroup\$ Excuse me. Fixed code a bit. \$\endgroup\$ – InfernumDeus Jan 23 '18 at 6:22
  • \$\begingroup\$ Are you sure that your code now works as intended or do you need to edit the code in question some more? \$\endgroup\$ – Heslacher Jan 23 '18 at 6:55
  • \$\begingroup\$ Yes. I tested everything except the way it handles exceptions. Not sure how to properly model cases that lead to this exact exceptions. \$\endgroup\$ – InfernumDeus Jan 23 '18 at 7:16
  • 1
    \$\begingroup\$ I will not edit code from this point. \$\endgroup\$ – InfernumDeus Jan 23 '18 at 7:23
6
\$\begingroup\$

You have multiple problems here:

  • For the case that an exception is thrown and swallowed you get a NullReferenceException at iterating over the filesInCurrent.

  • If the method would be called using C:/Windows/ (which is a valid path although it uses a slash instead of a backslash) the if condition would never result in true.

  • The if (folders.Count != 0) currentFolder = folders.Dequeue(); in addition with the continue can lead to problems. Assume the next folder in the queue would be a valid folder like C:\XFiles by calling Dequeue() and continue the folder would silently be swallowed.


  • Fixing the NullReferenceException is the easiest task. Just put the iterating over the filesInCurrent and foldersInCurrent inside the try block. By doing this you could place the declaration into the try as well.

  • Instead of checking if the currentFolder.Contains() a not wanted path, you should just refuse to add the paths while iterating over foldersInCurrent.

  • Instead of having the optimizer.IsSupported(f) which had former been a SomeLib.IsSupported() you should pass a Action<string, bool> which will just handle this. This will make your method future proof if e.g. the decision will be made to use a different lib.

  • To overcome the / vs \ problem you should create a DirectoryInfo() object using the root as parameter. By enqueueing the FullName property of the DirectoryInfo object any found subdirectory will have the \ as path separator.

  • The code should take advantage of the List<T>.AddRange() method instead of iterating over the files and adding them one by one.

  • If you later decide to exclude different paths it would be good to extract checking of the validness of the folders to a separate method which could be passed as Action<string, bool> parameter as well.

  • Parameters of public methods should always be ckecked for validness, hence a check for null and for emptiness of the root should be implemented.

  • One could use the var type if the right hand side of the assignment makes the type obvious.

  • Ommiting braces {} although tehy might be optional can lead to hidden and therfor hard to find bugs. I would like to encourage you to always use them.

  • Underscore prefixed variablenames are usually only for class-level variables.

  • Passing a root which doesn't exists should be checked at the beginning of the method as well. By returning early you won't need to catch a DirectoryNotFoundException.


Implementing the mentioned points will lead to

public static List<string> FindSupportedFiles(string root, Func<string, bool> isFileValid,Func<string,bool> isFolderValid)
{
    if (root == null) { throw new ArgumentNullException("root"); }
    if (string.IsNullOrWhiteSpace(root)) { throw new ArgumentException("The passed value may not be empty or whithespace", "root"); }

    var files = new List<string>();


    var rootDirectory = new DirectoryInfo(root);
    if (rootDirectory.Exists() == false) { return files; }

    //overcome problem about slash and backslash with using Contains()
    root = rootDirectory.FullName;
    if (isFolderValid(root) == false) { return files; }

    var folders = new Queue<string>();
    folders.Enqueue(root);
    while (folders.Count != 0)
    {
        string currentFolder = folders.Dequeue();

        try
        {
            var currentFiles = Directory.EnumerateFiles(currentFolder, "*.*").Where(f => isFileValid(f));
            files.AddRange(currentFiles);
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }

        try
        {
            var currentSubFolders = Directory.GetDirectories(currentFolder).Where(f => isFolderValid(f));
            foreach (string current in currentSubFolders)
            {
                folders.Enqueue(current);
            }
        }
        // Ignore this exceptions
        catch (UnauthorizedAccessException) { }
        catch (PathTooLongException) { }

    }
    return files;
}

private static bool IsFolderValid(string path)
{

    return (path.Contains(":\\Windows") ||
             (path.Contains(":\\Users") &&
              path.Contains("\\AppData"))) == false;
}  

and could be called like so

var optimizer = new ImageOptimizer();
var supportedFiles = FindSupportedFiles(path, optimizer.IsSupported, IsFolderValid);
\$\endgroup\$
  • \$\begingroup\$ Thank you! I see quite a few good practices that I should learn from your advices. \$\endgroup\$ – InfernumDeus Jan 23 '18 at 8:32
  • 1
    \$\begingroup\$ +1. Regarding DirectoryNotFoundException, what if some other process just deleted the folder you're about to scan? I'd also add that the folder validity check is brittle: it also matches things like C:\WindowsCleaners and D:\Userstuff\AppDatareader. \$\endgroup\$ – Pieter Witvoet Jan 23 '18 at 8:48
  • \$\begingroup\$ Pretty good point. I will look for proper solution later. I thinkg there should be some build in way in .Net. \$\endgroup\$ – InfernumDeus Jan 23 '18 at 8:54
  • \$\begingroup\$ @PieterWitvoet good points. Will leave it to the OP to integrate ;-) \$\endgroup\$ – Heslacher Jan 23 '18 at 9:59
  • 1
    \$\begingroup\$ Most prominent example: blog.codecentric.de/en/2014/02/curly-braces \$\endgroup\$ – Heslacher Jan 23 '18 at 11:15

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.