6
\$\begingroup\$

This method returns the list of files (absolute path) in a folder (or tree). It allows filtering by extensions or filenames.

The method receives the following parameters:

  • string path: folder path to scan for files.
  • string[] exclude: can contain filenames such as "read.me" or extensions such as "*.jpg".
  • SearchOption searchOption: TopDirectoryOnly to scan only the specified folder or AllDirectories to scan tree folder under the specified path.

The method return files that doesn't appear in exclude array and a) hasn't extension, or b) extension doesn't appear in exclude array.


    public static IEnumerable<string> GetFiles(string path, string[] exclude, SearchOption searchOption = SearchOption.AllDirectories)
    {
        IEnumerable<string> files = Directory.EnumerateFiles(path, "*.*", searchOption);
        var resultFiles = new List<string>();

        if (exclude.Length > 0)
        {
            foreach (var filename in files)
            {
                string extension = Path.GetExtension(filename);

                if (Array.IndexOf(exclude, Path.GetFileName(filename)) >= 0)
                {
                    continue;
                }

                if (string.IsNullOrEmpty(extension) || Array.IndexOf(exclude, "*" + extension) < 0)
                {
                    resultFiles.Add(filename);
                }
            }
        }

        return resultFiles;
    }
\$\endgroup\$
  • 2
    \$\begingroup\$ I'd recommend moving EnumerateFiles inside the true block of (exclude.Length > 0). There's no reason to fetch file names otherwise. Also your check could be ((exclude != null) && (exclude.Length > 0)). \$\endgroup\$ – Rick Davin Apr 22 '15 at 16:01
  • \$\begingroup\$ @RickDavin why not flesh it up just a little, and then post that as an answer? \$\endgroup\$ – Mathieu Guindon Apr 22 '15 at 16:16
  • \$\begingroup\$ @RickDavin if exclude has no elements, the result should be the same as EnumerateFiles, so I can't move it inside the true block. I'll add the check against null. \$\endgroup\$ – segarci Apr 22 '15 at 16:17
  • \$\begingroup\$ @segarci Your code has written does not reflect what you said. If exclude has no elements, then resultFiles, which was initialized a new List<string>, is simply returned without referencing anything from EnumerateFiles. \$\endgroup\$ – Rick Davin Apr 22 '15 at 17:28
  • \$\begingroup\$ @RickDavin You're right man! That's a bug. \$\endgroup\$ – segarci Apr 22 '15 at 17:47
6
\$\begingroup\$

You're already returning an IEnumerable<T> so you might as well turn it into an iterator block. Discard the local resultFiles and simply use yield return filename.

MSDN on yield


I don't like that both filenames and extensions are in the same array. If you have 1 extension and 5000 files that have to be excluded, it will iterate over 5000 files (if you have bad luck) for no reason.


I would consider using the .Contains() extension method: it's just more expressive than Array.IndexOf and should be no difference in performance.


I prefer string.IsNullOrWhiteSpace since that also takes care of.. well.. whitespace. You never know if the pesky QA departement tries something like that.


IEnumerable<string> files = Directory.EnumerateFiles(path, "*.*", searchOption);

if (exclude.Length > 0)
{
}

return resultFiles;

If there is nothing to exclude, you won't include anything either.


A new implementation might look like this:

public static IEnumerable<string> GetFiles(string path, IEnumerable<string> excludedFiles, IEnumerable<string> excludedExtensions, SearchOption searchOption = SearchOption.AllDirectories)
{
    var files = Directory.EnumerateFiles(path, "*.*", searchOption);

    foreach(var file in files)
    {
        if(excludedFiles.Contains(file))
        {
            continue;
        }

        if(excludedExtensions.Contains(Path.GetExtension(file)))
        {
            continue;
        }

        yield return file;
    }
}
\$\endgroup\$
6
\$\begingroup\$

A few things I noticed:

  • If the exclude array is empty you return an empty list instead of every file.

  • reversing your logic would allow you to check everything in one statement.

  • Since you're returning an IEnumerable<string> already, using the yield operator would make sense here.

  • Accepting an IEnumerable<string> instead of an string[] for the exclude parameter, gives the user more options. Also this allows you to leverage the LINQ extensions better.

Your code could look something like this:

public static IEnumerable<string> GetFiles(string path, IEnumerable<string> exclude, SearchOption searchOption = SearchOption.AllDirectories)
{
    IEnumerable<FileInfo> files = new DirectoryInfo(path).EnumerateFiles("*.*", searchOption);
    foreach (var filename in files)
    {

        if (!exclude.Any(x => x == filename.Name || (x.StartsWith("*") && x.Contains(filename.Extension))))
        {
            yield return filename.Name;
        }

    }
}
\$\endgroup\$
  • \$\begingroup\$ @RobH What exactly is meant by "expressive" here? I generally try to avoid using the ! operator in the way that you say is more expressive, because I've seen issues with people not noticing it is there at first, and failing to understand that the condition is being checked against false. \$\endgroup\$ – Zack Apr 22 '15 at 20:21
  • \$\begingroup\$ @Zack -FirstOrDefault implies that you want the first element in the sequence which is satisfied by your predicate. If none match, you get the default value. Any implies that you want to know if anything matches. That's the part that's more expressive. I haven't worked with anyone who has missed the not operator at the beginning of an expression... Could invert it and use continue if that's a problem though. \$\endgroup\$ – RobH Apr 23 '15 at 7:53
3
\$\begingroup\$

The most important thing is that your code does not do lazy enumeration, which means it will load every filenames in your whole drive into memory if you use this method on root directory. To avoid that, the method should either return filtered IEnumerable<string> from LINQ method or using yield return. Using LINQ is more convenient:

private static bool IsExcluded(string fileName, string[] exclude) {
    // if not excluded by filename
    if (exclude.Contains(Path.GetFileName(fileName))) return true;
    string extension = Path.GetExtension(fileName);
    return extension == null || exclude.Contains("*" + extension);
}

public static IEnumerable<string> GetFiles(string path, 
    string[] exclude = null, 
    SearchOption searchOption = SearchOption.AllDirectories)
{
    // I prefer using var keyword for any IEnumerable
    var files = Directory.EnumerateFiles(path, "*.*", searchOption);
    if (exclude != null && exclude.Length > 0) {
        files = files.Where(f => !IsExcluded(f, exclude));
    }
    return files;
}

I separate exclusion checking into a new method because it is easier to verify that the filter work correctly rather than doing it inline.

\$\endgroup\$
-2
\$\begingroup\$

Everything that you have written there can be done with a call to: Directory.GetFiles(string, string, SearchOption)

\$\endgroup\$
  • \$\begingroup\$ This is more of a comment than a review of OP's code. Maybe OP didn't know about Directory.GetFiles, or maybe OP is purposely reimplementing it - in any case there's nothing wrong with reinventing the wheel if it's for the heck of it. \$\endgroup\$ – Mathieu Guindon Apr 22 '15 at 15:35
  • 3
    \$\begingroup\$ It's not exactly reinventing the wheel. The big difference here is the exclude, which is something that neither GetFiles() nor EnumerateFiles() currently has. Note the OP uses EnumerateFiles() which is preferred in many ways. \$\endgroup\$ – Rick Davin Apr 22 '15 at 15:45
  • \$\begingroup\$ @Hosch250 I put the link in. This answer didn't enter the VLQ queue because it had a link ;) \$\endgroup\$ – Mathieu Guindon Apr 22 '15 at 15:59
  • 1
    \$\begingroup\$ @d347hm4n see here for an example of a great answer that suggests replacing OP's code with a BCL call. \$\endgroup\$ – Mathieu Guindon Apr 22 '15 at 16:02
  • \$\begingroup\$ GetFiles look at the old and new naming convention for files. Searching for *.txt will also return *.txt123 files. Looping and looking at the extension is still needed. \$\endgroup\$ – the_lotus Apr 22 '15 at 19:42

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.