Finding the next available filename 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 &lt; 100; ++i)
    {
        if (!File.Exists(nextName))
        {
            return nextName;
        }
        nextName = $"{fileName}_{i}.png";
    }

    throw new ApplicationException("Unable to get free filename");
}
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 &lt;=; if you start counting from 0, then &lt; 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 &lt;= 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.