Please improve my novice c++ code

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 look odd to me -- opinion only, but in all the code I've ever seen, those are rare

---

The noop statements on argv and argc jump out as odd. (I'm also not sure about the validity of it). 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.

---

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 would either return a copy of the path or a const reference. (Also, if you go the immutable route, you should make the properties const just to make sure they're never mutated, including internally.)

---

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. If you're going to write it explicity, you might as well go all the way and do the assignment operator too. I would probably just omit the copy constructor and use the default one though.

Oh, and also 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.

---

The performance of this is negligible, but I tend to store the result of things like end():

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

(It's worth noting that my C++11 experience is super limited, so that might be invalid syntax.)

---

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 might 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.

---

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;