generate a file name with format Name(Number).extension

Question

I am writing a program which downloads a file from a URL. Because the name of the downloaded file depends on the url, there is a risk of duplication if the user downloads the same file twice.

For example if the url is : http://www.example.org/myfile.zip The downloaded file name will be myfile.zip. and if the user downloads it again the name will be myfile(1).zip

To achieve this, I wrote the following code :

    //A file already exist, we use the usual name   
    //but add a number before the extension like "Name(X).extension" X being a number

    //Append the number just before the file extension
    auto pos = name.find_last_of(".");
    std::string nameWithoutExt = name.substr(0, pos);
    std::string extension = name.substr(pos);

    std::ostringstream possibleName;
    int i = 1;
    do
    {
        //Clear the string stream
        possibleName.str("");
        possibleName.clear();
        possibleName<< nameWithoutExt << "(" << std::to_string(i) << ")" << extension;
        ++i;

      //Check if a file with the possible name exists 
    } while (std::experimental::filesystem::exists(builder.str()));

    name = builder.str();

This solution does not look optimal to me, because it may require a lot of calls to std::experimental::filesystem::existsfunction. Are there any ways to improve this?

Answer 1

There are a couple of improvements that I think you might make to this code.

Provide complete code to reviewers

This is not so much a change to the code as a change in how you present it to other people. Without the full context of the code and an example of how to use it, it takes more effort for other people to understand your code. This affects not only code reviews, but also maintenance of the code in the future, by you or by others. One good way to address that is by the use of comments. Another good technique is to include test code showing how your code is intended to be used.

Use standard functions where appropriate

Since you're already using the experimental/filesystem routines, why not make better use of them? Here is a function called uniqueName which shows one way to do that using your current strategy:

fs::path uniqueName(const std::string &name) {
    fs::path possibleName{name};
    auto stem = possibleName.stem().string();
    auto ext = possibleName.extension().string();
    for (int i=1; fs::exists(possibleName); ++i) {
        std::ostringstream fn;
        fn << stem << "(" << i << ")" << ext;
        possibleName.replace_filename(fn.str());
    }
    return possibleName;
}

As suggested by the name of the function, this routine takes the name of a file and either returns it unaltered if no such file exists in the current directory or returns an altered filename as per your schema. Be aware that after this function is called, some other process could create a file with the same name.

Create a list and use it

As shown in the review by @yuri, you could create a list and then use that. It has the same potential problem noted above in that another process could create additional files after the list is created. This version does not require any regex:

std::unordered_set<std::string> create_file_list()
{
    std::unordered_set<std::string> m{};
    for (const auto item : fs::directory_iterator{fs::current_path()}) {
        m.emplace(item.path().filename().string());
    }
    return m;
}

This creates a list of just the filenames (stripping the path). This can then be used in a minor variation of the routine shown above. Note that only a single line is different.

fs::path uniqueName(const std::string &name, const std::unordered_set<std::string> files) {
    fs::path possibleName{name};
    auto stem = possibleName.stem().string();
    auto ext = possibleName.extension().string();
    for (int i=1; files.find(possibleName.string()) != files.end(); ++i) {
        std::ostringstream fn;
        fn << stem << "(" << i << ")" << ext;
        possibleName.replace_filename(fn.str());
    }
    return possibleName;
}

Answer 2

Based on the idea of @JulienRousé you could populate a map with the local filenames and how often a certain file exists. Then you can later check any prospective new files against the map and directly take the number of occurences and add it to the new filename.

An implementation could look like this:

#include <experimental/filesystem>
#include <fstream>
#include <sstream>
#include <iostream>
#include <string>
#include <regex>
#include <map>

namespace fs = std::experimental::filesystem;

using mymap = std::map<std::string, size_t>;

void create_file_list(mymap &map)
{
    std::regex rgx{R"(^([^(.]+))"};

    for (const auto &file : fs::directory_iterator(fs::current_path()))
    {
        std::smatch match;
        std::string fn = file.path().string();
        std::regex_search(fn, match, rgx);
        // note that we are using match[1] because match[0] holds the entire string we matched against
        ++map[match[1].str()];
    }
}

std::string check_filename(const std::string &file, mymap &map)
{
    std::regex rgx{R"(^([^(.]+)(\..+?)$)"};
    std::smatch match;
    std::regex_search(file, match, rgx);
    std::string cur_filename = match[1].str();
    std::string ext = match[2].str();

    if (map.count(cur_filename))
    {
        std::ostringstream new_filename;
        new_filename << cur_filename << "(" << map[cur_filename] << ")" << ext;
        ++map[cur_filename];
        return new_filename.str();
    }

    return {};
}

int main()
{
    mymap files;

    create_file_list(files);

    // get a filename from the site and test it against our map
    // and then do something with it
    std::string newfile = "foo.ext";
    std::string new_filename = check_filename(newfile, files);
}

Please note that I only made sure this compiles. I haven't tested this exhaustively. It also assumes filenames have exactly one dot (.) character in them and that duplicates will always be marked with "(x)" before the dot. Checking if there is a valid regex match has been omitted for clarity.