Reading a list of file paths and adding them to a map

Question

I have many thousands of image files spread around various places and would like to copy them into one directory. However, may of the files are duplicates with the same names (often thumbnails or reduced resolution etc). I want to delete all files that have the same name except for the largest.

The following code reads a list of these file paths (fullpath/name). It splits the filename from the path, determines the size of the file and adds each filename and its size to a list (map). Where a name is found that is already in the list, the larger of the two is kept in the list and the smaller is 'deleted' (by which I mean emitting the command "rm path/to/file").

I know this is naive C++, because I am a novice (though not at C). Any help making it less naive would (or just better) is appreciated.

#include <iostream>
#include <string>
#include <sys/stat.h>
#include <map>

using std::map;
using std::string;

class filedata {
    std::string path;
    size_t size;
public:
    filedata (void) : path(""), size(0) { }
    filedata (const string& p, size_t s) : path(p), size(s){ }
    filedata (const filedata& f) : path(f.path), size(f.size) { }

    size_t filesize(void) {
        return size;
    }

    string& filepath(void) {
        return path;
    }

    std::ostream& print(std::ostream& os) const {
        os << size << " " << path << "\n";
        return os;
    }
};

std::ostream& operator<<(std::ostream& os, const filedata& f)
{
    return f.print(os);
}

static size_t file_size(const string& s)
{
    struct stat st;
    if (stat(s.c_str(), &st) == 0) {
        return st.st_size;
    }
    return 0;
}

static void delete_file(const string& s)
{
    std::cerr << "rm \"" << s << "\"\n";
}

static bool file_name(const string& path, string& name)
{
    size_t pos = path.rfind('/');
    if (pos == string::npos) {
        return false;
    }
    name = path.substr(pos+1);
    return true;
}

int main(int argc, char **argv)
{
    (void) argc;
    (void) argv;

    string s;
    map<string, filedata> files;

    while (getline(std::cin, s)) {
        string name;
        if (file_name(s, name) == false) {
            break;
        }

        size_t size = file_size(s);
        if (size == 0) {
            delete_file(s);
        }
        else if (files.count(name) > 0) {
            filedata f(files[name]);
            if (f.filesize() <= size) {
                delete_file(f.filepath());
                files[name] = filedata(s, size);
            }
        }
        else {
            files[name]= filedata(s, size);
        }
    }

    for (auto i = files.begin(); i != files.end(); ++i) {
        std::cout << i->first << " "  << i->second;
    }
    return 0;
}

Answer 1

Looks good overall (though I'm by no means a C++ expert).

In coming brain-dump though:

---

void argument lists are rare in C++. Yes, in C, int c() and int c(void) are different, but in C++, that's not the case. In C++, int c() is implicitly the same declaration as int c(void).

---

The no-op statements on argv and argc jump out as odd. If you did that so a warning of an unused variable won't be issued, you could always use one of the alternate main declarations (i.e. int main().

---

Opinion again, but I'm not a fan of comparison to booleans: if (file_name(s, name) == false). It reads more easily as if (!file_name(s, name))

---

This might be on purpose, but the path property of filedata can be mutated from outside:

filedata f("name", 10);
f.filepath() = "new";

I would probably make these objects immutable since they seem to be value objects (I think you probably intended to do this, just non-intuitive references got you).

You should return either by value or by const reference from filepath. Even though it will create a usually unnecessary copy, I highly suggest the value route. If you return a const reference, you run the risk of a dangling pointer. Consider this (bad) example:

filepath* f = new filepath("blah", 10);
const std::string& s = f->filepath();
delete f;
// Any use of s past here is undefined behavior

s is essentially a const pointer to f->path. This means that when f is released, s becomes invalid. Note that if you immediately create a copy of a string based on the constant reference [i.e. if you had std::string s = ...] this does not apply. That still creates a copy though, so you might as well return a value.

(Also, obligatory note: raw pointers have very few legitimate uses in C++. Avoid pointers if at all possible, and use a smart pointer when not. The only (possible) exception is low level container implementations.)

---

I'm not a fan of print style member functions. In this application this aversion doesn't make sense, but imagine if some third party library you used provided print methods for some of it's classes. Do you think it would print out in exactly the format you want? I prefer to use helper-type functions for printing rather than putting it as a member of the class. (Though once again, on this application since it's so small, it matters nothing at all.)

(Oh, and this once again is opinion.)

---

filedata (void) : path(""), size(0) { }
filedata (const string& p, size_t s) : path(p), size(s){ }
filedata (const filedata& f) : path(f.path), size(f.size) { }

There's a bit of redundancy here. Technically you could write this just as:

filedata (const string& p = "", size_t s = 0) : path(p), size(s){ }

Or if you wanted:

filedata (const string& p = string(), size_t s = size_t()) : path(p), size(s){ }

Though I'd probably go with the first.

The copy constructor doesn't need to be defined since it's the same as the implicit one. When using the same functionality as the implicit one, do not define one. The implicit one isn't prone to typos or a future failure to update the copier to reflect new properties.

---

I would give more descriptive names to p and s:

filedata (const string& path, size_t size) : path(path), size(size) { }

---

I'd probably go with something like uint64_t instead of size_t since size_t will probably be 32 bits on a 32 bit system. If you'd like to abstract away exactly what the type is, you could have a typedef ... size_type member of your class. Not only does that offer a more semantic meaning, it allows for easier future change (assuming consumers actually proper use the typedef...).

---

I tend to declare define the end of an iterator up front:

for (auto i = files.begin(), end = files.end(); i != end; ++i) {

Not only does it potentially offer a tiny performance advantage (though not really on any modern compiler), it has clearer meaning. With this version, there much more suggestion that end is not mutated inside of the loop (though it's still not guaranteed since end isn't const).

---

std::begin and std::end should be preferred in C++11. They have all the capabilities of the member-function versions plus a bit more.

---

stat failing probably shouldn't just return a size of 0. You might end up accidentally deleting a file if something weird happens. (Then again, most of the reasons stat would fail would also cause an attempt to delete a file to fail.)

---

If you had the input include the file sizes, you could reduce your program to pure text processing. I'm also not sure if your program should be emitting rm commands. Seems like a coupling of sorts. Then again, the output would just be transformed into rm commands anyway, so might as well do it directly I guess :).

---

The data put out on stdout seems to be debugging-esque information whereas the stderr data seems to be what you're actually after. Seems like these streams should be swapped.

---

I'd be tempted to use a reference instead of copying the item in the map:

filedata f(files[name]);

Could be:

filedata& f = files[name];

The performance difference is going to be non-existent; mostly just a (opinion-y) style thing. It would also allow you to simplify the reassignment from files[name] = ... to just f = ....

---

The filesize member of filedata can't permute size, so you might as well have the method be const. Same for filepath() if it wasn't meant to allow the perumtation of path (as mentioned earlier).

---

Another personal-style thing: I don't like implicit visibility scoping:

class filedata {
    std::string path;
    size_t size;

---

os << size << " " << path << "\n";
return os;

Can be simplified to:

return os << size << " " << path << "\n";