2
\$\begingroup\$

We're having a small debate in work about the following piece of code:

private string GetNextFileName(string fileName)
{
    int index = 1;

    string nextName = $"{fileName}_{index}.png";

    while (File.Exists(nextName))
    {
        nextName = $"{fileName}_{++index}.png";
    }

    return nextName;
}

Is there anything particularly dangerous about this piece of code? It sort of feels dodgy having the condition in a while loop.

Would it be better to use a for-loop with a breakout if the number gets too high?

e.g.

private string GetNextFileName(string fileName)
{
    int i = 1;
    string nextName = $"{fileName}_{i}.png";

    for (; i < 100; ++i)
    {
        if (!File.Exists(nextName))
        {
            return nextName;
        }
        nextName = $"{fileName}_{i}.png";
    }

    throw new ApplicationException("Unable to get free filename");
}
\$\endgroup\$

closed as off-topic by dfhwze, t3chb0t, Toby Speight, pacmaninbw, AJNeufeld Jul 29 at 17:20

This question appears to be off-topic. The users who voted to close gave this specific reason:

  • "Lacks concrete context: Code Review requires concrete code from a project, with sufficient context for reviewers to understand how that code is used. Pseudocode, stub code, hypothetical code, obfuscated code, and generic best practices are outside the scope of this site." – dfhwze, t3chb0t, Toby Speight, pacmaninbw, AJNeufeld
If this question can be reworded to fit the rules in the help center, please edit the question.

  • \$\begingroup\$ It is not thread-safe and is I/O-intensive. \$\endgroup\$ – Jesse C. Slicer Jul 16 at 15:01
  • 1
    \$\begingroup\$ Loop conditions should reveal the intent of the loop when possible. Use the condition from the first example and throw on exception. Whether it's best to limit the number of files is a separate question, not some universal moral constant, and it totally depends on what the filename is used for. \$\endgroup\$ – Oh My Goodness Jul 16 at 15:07
  • 1
    \$\begingroup\$ This question lacks concrete context. For instance, you are not showing us when you call this code. This method always assumes an index has to be added, which infers that you have checked File.Exists already before the method call. \$\endgroup\$ – dfhwze Jul 16 at 16:09
  • 1
    \$\begingroup\$ Why don't you tell us the use case for this? Do you require this template of file names or are temporary or random file names also ok? \$\endgroup\$ – dfhwze Jul 17 at 16:30
  • 1
    \$\begingroup\$ I also downvoted because it's not clear what happens when some file is deleted. You would then get the wrong filename from somewhere in the middle. \$\endgroup\$ – t3chb0t Jul 21 at 10:48
5
\$\begingroup\$

Stylistically, the for loop is more readable. As a general rule, if you start the loop at 1, then I would prefer the termination condition to use <=; if you start counting from 0, then < would be better.


The design of this function is conceptually flawed. It will return an unused filename, but presumably you will eventually want to create a file with the name. However, it is possible that someone else will have created a file with the same name, creating a conflict. This possible race condition means that you can never be certain that the filename returned by this function is actually unused.

The way to make that guarantee is to actually create the file, such that you effectively grab a reservation on that name. You can do that by calling File.Open(path, FileMode.CreateNew). The caller would have to delete the file if it doesn't want it.

private static string CreateNextAvailableFile(string fileNamePrefix)
{
    string name;
    for (int i = 1; i <= 99; ++i)
    {
        try
        {
            FileStream fs = File.Open(name = $"{fileNamePrefix}_{i}.png", FileMode.CreateNew);
            fs.Close();
            return name;
        }
        catch (IOException e)
        {
            // Did File.Open() fail due to name collision or another
            // reason? Unfortunately, no specific "FileAlreadyExists"
            // exception exists.  This is a heuristic, and can be
            // fooled by a race condition where the file was deleted
            // just now by someone else.
            if (!File.Exists(name)) throw;
        }
    }

    throw new ApplicationException("Unable to get free filename");
}

As the comment notes, .NET unfortunately has no way to tell whether the IOException was due to a filename collision or something else. The code above is therefore also vulnerable to a race condition, but it's better than your race condition, because:

  • This race condition is only triggered when there is an underlying I/O problem (such as insufficient permissions to create a file, or a read-only filesystem), and the file in question is deleted during the split-second between the File.Open() call and the exception handler.
  • This code fails in a safer way: if the race condition is triggered, it will throw an IOException. (Arguably, such an IOException would happen anyway, later on.) However, it's harder to tell what might be the consequences of your race condition: it might fail to detect a collision, and lead to data being overwritten.
\$\endgroup\$
  • 2
    \$\begingroup\$ There are some slight improvements to make: (1) FileStream comes with a Dispose that does more than just Close, so use a using block for the stream: referencesource.microsoft.com/#mscorlib/system/io/filestream.cs (2) you don't need to declare the exception e since you don't handle it \$\endgroup\$ – dfhwze Jul 17 at 4:38
  • \$\begingroup\$ Stylistically, the for loop is more readable. - Is this for (; i < 100; ++i) really more readable than this while (File.Exists(nextName))? I doubt. \$\endgroup\$ – t3chb0t Jul 21 at 10:22
  • 1
    \$\begingroup\$ I find this exception-driven brute-foce solution is terrible. Having several thousand files would make this the bottleneck #1. \$\endgroup\$ – t3chb0t Jul 21 at 10:38
  • \$\begingroup\$ This code will throw UnauthorizedAccessException if $"{fileNamePrefix}_{i}.png" exist as a directory. \$\endgroup\$ – tsh Jul 22 at 7:34
2
\$\begingroup\$

Personally I'm a fan of using Regex when it comes to parsing strings for the simplicity it provides. This way we can check for the next available file name without relying on loops. Only problem I see with this method is if the directory contains an absurd amount of files.

//the following regex pattern divides the filename into 3 groups:
// [1] directory + filename before the last underscore
// [2] the index or numeric part before the last dot
// [3] the extension of the file

private static Regex fileNamePattern = new Regex("(.*)_([0-9]*).(.*)"); 

//this of course takes for granted that the initial file name is correctly generated

private static void GetNextFileName(string fileName)
{
    string newFileName = fileName;

    if (File.Exists(newFileName))
    {
        var existingFileNames = Directory.GetFiles(Path.GetDirectoryName(fileName)).Where(x => fileNamePattern.IsMatch(x));
        string existingMaxIndex = existingFileNames.Max(x => fileNamePattern.Match(x).Groups[2].Value);
        int newMaxIndex = int.Parse(existingMaxIndex) + 1;
        newFileName = fileNamePattern.Replace(newFileName, m => $"{m.Groups[1].Value}_{newMaxIndex}.{m.Groups[3].Value}");
    }

    //here you should proceed to create the file instead of returning it, as @200_success explained
}
\$\endgroup\$
  • 1
    \$\begingroup\$ This could be cleaner but in general this is the coolest solution. \$\endgroup\$ – t3chb0t Jul 21 at 10:39
1
\$\begingroup\$

It appears that the string you pass into the method is a type of picture not really a filename, since the filename needs an index attached.

I'm not a big fan of either approach. I think using GetFiles and using the Length property is easier to see what's happening.

Something like this should work:

using System;
using System.IO;

    private static string GetNextFilename(string picName)
    {
        string fileName = $"{picName}_{1}.png";
        DirectoryInfo dir;  
        if(File.Exists(fileName))
        {
            FileInfo info = new FileInfo(fileName);
            dir = info.Directory;           
        }
        else
        {
           return fileName;
        }       
        int nextIndex = dir.GetFiles($"{picName}*.png").Length + 1;
        return $"{picName}_{nextIndex}.png";
    }
\$\endgroup\$

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