1
\$\begingroup\$

The following routine returns the file-names from the current working directory:

    public static List<string> GetRelativeFileNamesInTheCurrentDirectory(string[] extensions)
    {
        string curr = System.IO.Directory.GetCurrentDirectory();

        string[] files = Directory.GetFiles(curr);

        List<string> filesList = new List<string>();

        for (int i = 0; i < files.Length; i++)
        {
            FileInfo temp = new FileInfo(files[i]);

            foreach (var item in extensions)
            {
                if (temp.Extension == item)
                {
                    filesList.Add(temp.Name);
                }
            }
        }

        return filesList;
    }

Even though it works, I am not satisfied with the source code's arrangement.

How can I improve it?

\$\endgroup\$
1
  • \$\begingroup\$ What about a single line: return new DirectoryInfo(Environment.CurrentDirectory).EnumerateFiles().Where(file => extensions.Contains(file.Extension)).Select(file => file.Name).ToList();? \$\endgroup\$
    – Rufus L
    Mar 3 at 0:46

2 Answers 2

8
\$\begingroup\$

There are a staggering number of ways to achieve what you want. Before we get to that, let's have a quick critque of your code.

  • You should have a using System.IO;
  • Names do not need to be overly abbreviated. E.g. curr could be current or currentDirectory.
  • temp should be named file.
  • Your input signature is rigid in that only a string array is allowed.
  • Overall, the method you wrote is extremely specific as that the input must be an array, only the current directory is searched, and only a List is returned.

One philosophy I have adopted is to make code into smaller, more manageable methods that (1) favor flexibility over rigidity, and (2) can be re-used for more than the original specific use.

To show you just 1 of many alternatives, let's look at my many changes for smaller code.

I use EnumerateFiles instead of GetFiles. This could possibly be more performant, but it since it uses low memory, that is a full array is not materialized with IEnumerable, then you have more options with LINQ, etc.

The input parameter is now IEnumerable<string>. It could be an array, a list, a sorted list, etc.

Since you may repeatedly query the extensions, I think it works best as a HashSet. And, you want that to be a case-insensitive HashSet since you should treat ".txt", ".TXT", and ".Txt" as the same.

I return a List<FileInfo> because I have taken the time to fetch that and do not want to throw away some possible useful information. If I want a file name, then I can easily get that file name.

I can have shorter code thanks to LINQ.

My alternative:

public static IEnumerable<FileInfo> EnumerateFilesRelativeToCurrentDirectory(IEnumerable<string> fileExtensions)
           => EnumerateFilesRelativeToDirectory(new DirectoryInfo(Directory.GetCurrentDirectory()), fileExtensions);

public static IEnumerable<FileInfo> EnumerateFilesRelativeToDirectory(DirectoryInfo directory, IEnumerable<string> fileExtensions)
{
    IEnumerable<FileInfo> files = new List<FileInfo>();

    HashSet<string> allowedExtensions = fileExtensions.ToHashSet<string>(StringComparer.OrdinalIgnoreCase);

    if (allowedExtensions.Count == 1)
    {
        files = directory.EnumerateFiles("*" + fileExtensions.First());
    }
    else if (allowedExtensions.Count > 1)
    {
        files = directory.EnumerateFiles().Where(file => allowedExtensions.Contains(file.Extension));
    }

    return files;
}

public static void EnumerateFilesExample()
{
    var fileExtensions = new List<string>() { ".txt", ".csv", ".json" };

    // You can foreach over streamingfiles without regard to Count of index/position.
    // If you want just the name and path, use the FileInfo.Name property.
    // If you want the whole path, use the FileInfo.FullName property.
    var streamingFiles = EnumerateFilesRelativeToCurrentDirectory(fileExtensions);

    // If you demand a Count or want to use positional index, then convert to a list.
    var listOfFiles = EnumerateFilesRelativeToCurrentDirectory(fileExtensions).ToList();

    // If you truly just want a list of the file names, you can use:
    var filenames = EnumerateFilesRelativeToCurrentDirectory(fileExtensions)
                    .Select(file => file.Name)
                    .OrderBy(name => name.ToLower())
                    .ToList();  
}

The method to filter by file extension now accepts different types of inputs and can be used against more than just the current directory. But I also have a convenient method to just check the current directory. And I do not need to materialize anything to a full list until I absolutely need it (and I would even question if I truly needed to ToList() it before doing so).

UPDATE

If you want the shortest, simplest of code, my earliest version to you (since edited) was:

public static IEnumerable<FileInfo> EnumerateFilesRelativeToDirectory(DirectoryInfo directory, IEnumerable<string> fileExtensions)
{
    HashSet<string> allowedExtensions = fileExtensions.ToHashSet<string>(StringComparer.OrdinalIgnoreCase);
    return directory.EnumerateFiles().Where(file => allowedExtensions.Contains(file.Extension));
}

One may note, such as @iSR5 in a comment below, that I use a return instead of yield return as one would expect for a method returning IEnumerable. I am under the impression for many years that there is a special use case where return is preferred. I have tried to find some authoritative link somewhere to backup my contention, but all I found was this SO answer from @LBushkin back in 2010:

Is there ever a reason to not use 'yield return' when returning an IEnumerable?

I also believe there is a limitation with using only return instead of yield return: you may only use it once, and there can be no other yield break or yield return used within the method.

\$\endgroup\$
6
  • 2
    \$\begingroup\$ you could simplify EnumerateFilesRelativeToDirectory by using yield return to reduce memory allocation. something like codeshare.io/0gOXZD \$\endgroup\$
    – iSR5
    Jan 22 at 0:56
  • \$\begingroup\$ @iSR5 Thanks for the comment. I have updated my answer to explain why I use return. \$\endgroup\$
    – Rick Davin
    Jan 23 at 16:31
  • \$\begingroup\$ if SE could give a chance of double voting, I wouldn't hesitate of given it to you. \$\endgroup\$
    – iSR5
    Jan 23 at 18:28
  • \$\begingroup\$ @iSR5 Thanks but if they allowed double voting, they would also allow double down-voting too. \$\endgroup\$
    – Rick Davin
    Jan 23 at 18:30
  • 1
    \$\begingroup\$ +1; there's nothing exciting about returning in an IEnumerale returning method: it's just a normal method in that case. A yield break or yield return statement changes the whole method to an iterator, and would be unnecessary in your example. returning in such a case may be preferable in terms of performance (because resources may be relinquished and the returned object may not actually be lazy); yielding may provide a better debugging experience; I'd prefer to return because it is simpler and guarantees there is nothing else yielded 'above'. \$\endgroup\$ Jan 25 at 10:27
1
\$\begingroup\$

I would rewrite the code a bit.
We use DirectoryInfo, which will issue FileInfo.
We also use the EnumerateFiles method so as not to store the intermediate result, since we do not need it.

static List<string> GetRelativeFileNamesInTheCurrentDirectory(string[] extensions)
{
    var dirInfo = new DirectoryInfo(Directory.GetCurrentDirectory());

    var files = new List<string>();

    foreach (var fileInfo in dirInfo.EnumerateFiles())
    {
        foreach (var item in extensions)
        {
            if (fileInfo.Extension == item)
            {
                files.Add(fileInfo.Name);
            }
        }
    }

    return files;
}

Look at modern File globbing in .NET.

static List<string> GetRelativeFileNamesInTheCurrentDirectory(string[] extensions)
{
    string curr = Directory.GetCurrentDirectory();
    
    var patterns = extensions.Select(x => "*" + x);

    Matcher matcher = new();
    matcher.AddIncludePatterns(patterns);

    var files = matcher.GetResultsInFullPath(curr);

    return files.Select(x => Path.GetFileName(x)).ToList();
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

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