Code Review Stack Exchange is a question and answer site for peer programmer code reviews. It's 100% free, no registration required.

Sign up
Here's how it works:
  1. Anybody can ask a question
  2. Anybody can answer
  3. The best answers are voted up and rise to the top

I find that I am often writing files where the names either:

  1. Need to be incremented (rather than overwriting a file that already exists in the directory with the same name)
  2. Need to be timestamped

And, so, I've created the following methods to do so:

public static string CreateDatedFileName(string directory, string filename, string DateTimeFormat = "yyyy-MM-dd", bool AppendNumberIfNameAlreadyExists = true)
{
    if (!CheckIfDirectoryExists(directory))
        throw new ArgumentException(string.Format("The specified directory - {0} - does not exist", directory));

    string formatedTemplate = "{0} - {1}.{2}";

    var fileNameWithoutExtension = filename.Split('.')[0];
    var fileExtension = filename.Split('.')[1];
    var DateValue = DateTime.Now.ToString(DateTimeFormat);

    string newname = String.Format(formatedTemplate, fileNameWithoutExtension, DateValue, fileExtension);
    string path = Path.Combine(directory, newname);

    if (!AppendNumberIfNameAlreadyExists)
        return Path.Combine(directory, newname);
    else
        return CreateFileName(directory, newname, true);
}

public static string CreateFileName(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)
{
    if (!CheckIfDirectoryExists(directory))
        throw new ArgumentException(string.Format("The specified directory - {0} - does not exist", directory));

    string formatedTemplate = "{0}{1}.{2}";

    var fileNameWithoutExtension = filename.Split('.')[0];
    var fileExtension = filename.Split('.')[1];

    string newname = String.Format(formatedTemplate, fileNameWithoutExtension, string.Empty, fileExtension);
    string path = Path.Combine(directory, newname);
    int AppendDigit = 0;

    while (AppendNumberIfNameAlreadyExists && CheckIfFilePathExists(path))
    {
        ++AppendDigit;
        newname = String.Format(formatedTemplate, fileNameWithoutExtension, "_" + AppendDigit, fileExtension);
        path = Path.Combine(directory, newname);
    }

    return path;
}

public static bool CheckIfDirectoryExists(string directory)
{
    return System.IO.Directory.Exists(directory);
}

public static bool CheckIfFilePathExists(string path)
{
    return System.IO.File.Exists(path);
}

I'm just curious to know if this is an efficient way to accomplish this and, if not, I'd really like to learn why so as to become a better coder overall.

share|improve this question
    
Do you need a permanent file name, or is this for a temporary file? – RubberDuck Jan 13 at 21:21
1  
Where's AppendNumberIfNameAlreadyExists? – Mat's Mug Jan 13 at 21:22
1  
@RubberDuck, this would be a permanent file name for saving data to disk. – John Bustos Jan 13 at 21:22
5  
Oh lol, sorry I missed it! this is why locals & parameters should be camelCase ;-) – Mat's Mug Jan 13 at 21:23
6  
Nothing to do with your question, and others may disagree, but I don't like the method name CheckIfDirectoryExists(). It doesn't make a lot of sense to me when used as !CheckIfDirectoryExists() - what's a "not check..."? If a method returns a bool I would prefer a name that makes "English" sense when used in an if statement. E.g. if (DirectoryExists("xxx")) or if (!DirectoryExists("xxx")) – eurotrash Jan 13 at 21:40

The only way to guarantee that the generated filename is truly unique is to actually create the file. Any other method is vulnerable to a race condition. Therefore, if you are serious about uniqueness, you should change

public static string CreateFileName(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)

to

/// Creates a file in the directory with the given filename
///
/// <param name="AppendNumberIfNameAlreadyExists">
/// If true, then a numbered suffix (starting from 0) is appended
/// to the filename if necessary to avoid a name clash.
///
/// If false, then an IOException is thrown if the file already exists.
/// </param>
public static FileStream CreateUniqueFile(string directory, string filename, bool AppendNumberIfNameAlreadyExists = true)
share|improve this answer
4  
I might add that CreateUniqueFile doesn't really make sense if AppendNumberIfNameAlreadyExists = false (should it even be an argument to the method?). At that point, it's simply CreateFile – Rob Jan 13 at 23:28

You're assuming that filenames can't contain a period/full stop . other than to separate the name and the extension:

var fileNameWithoutExtension = filename.Split('.')[0];
var fileExtension = filename.Split('.')[1];

That's not true: "my.file.txt" is a perfectly valid filename - your code appears to incorrectly drop the file extension and changes the extension to ".file". That's a pretty big bug.

You should use the methods available on the System.Io.Path class e.g. GetFileNameWithoutExtension rather than using Split.

share|improve this answer

Here are some immediate responses to your post:

  • Please follow name conventions for parameters – I was very confused trying to locate DateTimeFormat and some others, please do use camelCase all over unless we're talking about public properties.
  • Why the CheckIfDirectoryExists()? – I'm kind of confused as to why you've made this functions, when the original System.IO.Directory.Exists() is almost the same size? If you add using System, you do the IO.Directory.Exists() or IO.File.Exists() and it is actually shorter than your extra functions...
  • The two main functions are very similar – I would consider joining your two functions into one, and possibly have some sort of loop choosing what extension to use. One way to join these could be to add another parameter being the extension (which then would/could skip the splitting of the file name)
  • Alternate extensions variants – In the loop trying to get unique file endings, I would consider looping on yyyy-MM-dd, and then yyyy-MM-ddTHH:mm, and then possibly yyyy-MM-ddTHH:mm::ss. This instead of simply adding numbers. This would enable the file list to be alphabetically sorted correctly, and makes sense if you create multiple files at nearly the same time.
  • Simplify the first extension to origFilename_Extension.orgExt – I would suggest not to introduce spaces to filenames if it can be avoided, and I would suggest using underscore instead of the minus, -.
share|improve this answer

This does not directly relate to your question but I feel it's still relevant. The Windows API has a function for generating a guaranteed unique random file name (as in, a bunch of random characters and numbers).

This API is exposed in .NET via Path.GetRandomFileName: https://msdn.microsoft.com/en-us/library/system.io.path.getrandomfilename(v=vs.110).aspx

Example taken from said page:

namespace ConsoleApplication1
{
    class Program
    {
        static void Main(string[] args)
        {
            string result = Path.GetRandomFileName();
            Console.WriteLine("Random file name is " + result);
        }
    }
}

/*

 This code produces output similar to the following:

 Random file name is w143kxnu.idj
 Press any key to continue . . .

 */

Again, this doesn't help with your requirement for date and time stamping but I thought it was worth mentioning.

share|improve this answer

Your Answer

 
discard

By posting your answer, you agree to the privacy policy and terms of service.

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