Copying jpg files between two folders

Question

• I have a two directories, input and output.

• input is a flat directory containing, among others, some .jpg files.

• output has nested subdirectories and contains .jpg files with same names as these in input.

• There can be some names in input missing in output.

• Names in output can be duplicated in different subdirectories.

An example structure:

$ tree input/
input/
├── a
├── b
├── c
├── d
├── e.jpg
├── f.jpg
├── h
├── i
├── j.jpg
└── z.jpg

0 directories, 10 files
$ tree output
output
├── 1
│   └── 2
│       └── 3
│           └── e.jpg
├── A
│   └── B
│       └── f.jpg
├── O
│   └── j.jpg
└── X
    └── Y
        └── Z
            └── j.jpg

9 directories, 4 files

The task is to overwrite all .jpg files in output directory with these from input based on their names.

---

Python version:

#!/usr/bin/env python
# -*- coding: utf-8 -*-

import collections
import os
import shutil
import sys


def get_paths(root):
    paths = collections.defaultdict(list)
    for path, subdirs, files in os.walk(root):
        for f in files:
            if os.path.splitext(f)[1] == '.jpg':
                paths[os.path.basename(f)].append(os.path.join(path, f))
    return paths


def main():
    if len(sys.argv) != 3:
        msg = 'python {} path/to/input/dir path/to/output/dir'
        print(msg.format(sys.argv[0]))
        sys.exit(1)

    input_dir, output_dir = sys.argv[1], sys.argv[2]
    input_paths, output_paths = get_paths(input_dir), get_paths(output_dir)

    for filename, input_path in input_paths.items():
        for output_path in output_paths.get(filename, []):
            shutil.copy(input_path[0], output_path)


if __name__ == '__main__':
    main()

Usage:

$ cat input/e.jpg input/f.jpg input/j.jpg 
e
f
j
$ cat output/1/2/3/e.jpg output/A/B/f.jpg output/X/Y/Z/j.jpg output/O/j.jpg 
X
X
X
X
$ python test.py input/ output/
$ cat output/1/2/3/e.jpg output/A/B/f.jpg output/X/Y/Z/j.jpg output/O/j.jpg 
e
f
j
j

---

C++14 version:

#include <map>
#include <list>
#include <iostream>
#include <boost/filesystem.hpp>

auto get_paths(boost::filesystem::path root) {
    std::map<
        const boost::filesystem::path,
        std::list<boost::filesystem::path>
    > paths;

    boost::filesystem::recursive_directory_iterator it(root);
    const boost::filesystem::path jpg(".jpg");
    for(; it != boost::filesystem::recursive_directory_iterator(); ++it) {
        if(boost::filesystem::is_regular_file(*it) && boost::filesystem::extension(*it) == jpg){
            paths[it->path().filename()].push_back(it->path());
        }
    }
    return paths;
}

int main(int argc, char** argv) {
    if(argc != 3) {
        std::cerr << argv[0] << " path/to/input/dir path/to/output/dir" << std::endl;
        return 1;
    }

    boost::filesystem::path input_dir(argv[1]);
    boost::filesystem::path output_dir(argv[2]);

    auto input_paths = get_paths(input_dir);
    auto output_paths = get_paths(output_dir);

    for(auto const& input_pair: input_paths) {
        const auto output_pair = output_paths.find(input_pair.first);
        if(output_pair != output_paths.end()) {
            for(auto const& output_path: output_pair->second) {
                boost::filesystem::copy_file(
                    input_pair.second.front(),
                    output_path,
                    boost::filesystem::copy_option::overwrite_if_exists
                );
            }
        }
    }
}

Usage:

$ cat input/e.jpg input/f.jpg input/j.jpg 
e
f
j
$ g++ test.cpp -o test -lboost_system -lboost_filesystem -std=c++14
$ cat output/1/2/3/e.jpg output/A/B/f.jpg output/X/Y/Z/j.jpg output/O/j.jpg 
X
X
X
X
$ ./test input/ output/
$ cat output/1/2/3/e.jpg output/A/B/f.jpg output/X/Y/Z/j.jpg output/O/j.jpg 
e
f
j
j

Answer 1

I can only comment on the python half. Broadly speaking, I'd say this is excellent already. Your code is clean, readable and uses standard libraries well. Your algorithm is reasonable, and will scale pretty well. Consider all of the below tiny nits.

if os.path.splitext(f)[1] == '.jpg':
    paths[os.path.basename(f)].append(os.path.join(path, f))

os.path.basename(f) is always just f here, because you're iterating over a flat file list. For the extension, I've seen ".jpeg", ".JPG", etc. You may want to take a second look at this whole program thinking carefully about case-sensitivity.

To make this more readable, I might rewrite this with intermediate variable names, but it's a matter of taste. _ is a traditional python name for "we don't care about this variable and will never use it".

for dir, subdirs, files in os.walk(root):
    for f in files:
        _, ext = os.path.splitext(f)
        file_path = os.path.join(dir, f)
        paths[f].append(file_path)

I would make

msg = 'python {} path/to/input/dir path/to/output/dir'

slightly more descriptive:

USAGE = '{program_name} path/to/input/dir path/to/output/dir'

You can chmod +x this program and run it without 'python' as the first argument, on Mac and Linux and least (not sure about Windows).

input_dir, output_dir = sys.argv[1], sys.argv[2]

might be clearly written to remind that it's an exhaustive list as:

_, input_dir, output_dir = sys.argv

Because you're using defaultdict,

for output_path in output_paths.get(filename, []):

can be just

for output_path in output_paths[filename]:

And finally, I'll point out that you're collecting all files of a given name in the input path, but only using the first. That seems fine--I think I prefer readability over efficiency. But perhaps you should be checking that the content is actually identical for the input files?

Answer 2

Since the Python version has already gotten a review, I'll review the C++ version.

Prefer vector to list

As is conveyed in the answers to this question there are very few circumstances in which a std::list is a better choice than a std::vector and this doesn't appear to be one of those.

Use namespace to shorten lengthy namespaces

I find that writing namespace fs = boost::filesystem; makes it easier to read the code. In this case, we could put it at the top of the file since it's a fairly short and simple program. In larger programs with multiple modules, it would generally be better to put such declarations only inside functions or in non-header implementation files (that is, in .cpp files but not in .h files).

Use const where practical

There are some uses of const here that are correct, but more could be done. In particular, the argument for get_paths should probably be const fs::path& root.

Consider the utility of single-use variables

The code currently contiains these lines:

boost::filesystem::path input_dir(argv[1]);
boost::filesystem::path output_dir(argv[2]);

auto input_paths = get_paths(input_dir);
auto output_paths = get_paths(output_dir);

I like named variables and constants in general, but in this case, input_dir and output_dir, both of which are only used once, add little value in my view. Instead, I would reduce this to these two lines:

auto input_paths = get_paths(argv[1]);
auto output_paths = get_paths(argv[2]);

Use range for to simplify your code

The use of an explicit iterator in get_paths makes the code longer than it needs to be. I think it's clearer to write this using a range for instead:

for (const auto& dir : fs::recursive_directory_iterator(root)) {
    if (fs::is_regular_file(dir) && dir.path().extension() == ".jpg") {
        paths[dir.path().filename()].emplace_back(dir.path());
    }
}
return paths;

Check function return values

The file copy could fail and throw an exception. This could happen, for example, if you invoke the program with the same directory as input and output which might also be something you could check for. I'd suggest catching the exception and notifying the user what happened. Depending on your need and the particular error, it might also be reasonable to continue after a single failure.

Consider writing for C++17

It's still the case that the Boost implementation is more widely available than the standard C++17 std::filesystem version, but with the use of the for loop shown above, there's only one place in your code where they now differ. While Boost uses

fs::copy_option::overwrite_if_exists

the C++17 version of this would be

fs::copy_options::overwrite_existing

Think of the user

There are a couple of enhancements (other than error checking) which might be useful for this program. One thing, would be to allow the user to do this same operation on file extensions other than ".jpg" which would only require a minor change. The other is to notify the user if there are duplicate names in the input. That is, if there are two files named "j.jpg" the program currently and silently only uses the last one found.