Adding/changing ID3 tags to media files with "artist - title" format

Question

This code puts or changes ID3 tags to media files based on their file names. It assumes the files are in this format:

morbid angel - maze of torment.mp3

The resulting tag would be:

Artist: morbid angel

Title: maze of torment

I use boost::filesystem to iterate through the directory and taglib to manipulate the tags.

#include <boost/filesystem.hpp>
#define TAGLIB_STATIC
#include <taglib/fileref.h>
#include <iostream>
#include <string>
#include <vector>

using namespace boost::filesystem;
using namespace TagLib;

void changeTag(const path& p) {
    std::string filename = p.filename().string();
    FileRef file(p.string().c_str());
    std::string artist = filename.substr(0,filename.find(" -"));
    Tag* t = file.tag();
    t->setArtist(artist);
    t->setTitle(filename.substr(artist.size()+3, filename.size()-artist.size()-7));
    file.save();
    std::cout << t->artist() << " - " << t->title() << " saved" << '\n';
}

void extractFiles(const std::string& p) {
    std::vector<path> files;
    std::vector<std::string> errors; // for files that might cause errors
    try {
        path filepath = p;
        for (auto& i : directory_iterator(filepath)) {
            changeTag(i.path());
        }
    } catch(const filesystem_error& e) {
        errors.push_back(e.what()); // output later after all files have been processed
    }
    if (errors.size() > 0) {
        std::cout << "Some operations failed with errors:" << '\n';
        for (auto& i : errors) {
            std::cout << i << '\n';
        }
    }
}

int main() {
    std::string dir;
    while (true) {
        std::cout << "Directory: Enter 'x' to close: ";
        std::cin >> dir;
        if (dir == "x") {
            break;
        }
        extractFiles(dir);
    }
}

How can I improve this code? How can I make this faster, cleaner and better?

Answer 1

Here are some things I see that may help you improve your code.

Add error checking

In your changeTag routine, a segfault can occur if there is no file. Add this just after the FileRef line:

if(!file.isNull() && file.tag()) {

Separate concerns

The code would be a little more clear if the changeTag routine was split into two parts. In particular, one could extract artist and title, and the other could apply those. It may also be reasonable to make extractFiles return the vector of errors rather than having it emit those errors to std::cout. The calling program might be able to do something different with those, such as if it were a GUI.

Use command line parameters

Instead of prompting for a directory name, it would be better to have that as a command line parameter. That would also simplify the control structure of your program as mentioned in the next comment.

Try to consolidate loop exit conditions at the top

Prefer a for loop with the actual exit condition to a while(true) loop. Because the reader is forced to search for the loop terminating condition, it's more effort to understand the latter construct. This code, for example could be structured like this:

int main() {
    std::string dir;
    std::cout << "Directory: Enter 'x' to close: ";
    for (std::cin >> dir; dir != "x"; std::cin >> dir) {
        extractFiles(dir);
    }
}

This also eliminates a break statement. A break can also contribute to difficulties understanding a loop because again, the programmer reading the code has to search for them.

Consider the user

It might be nice to have an option for a "dry run" of the program. That is, have it print what it would have done without actually altering any files. This would help the user have confidence that the program was doing only and exactly what was intended.